feat(blob): blobsub #3539
feat(blob): blobsub #3539
Conversation
distractedm1nd
requested review from
renaynay,
Wondertan,
vgonkivs,
walldiss,
ramin and
cristaloleg
as code owners
|
|
||
| for { | ||
| select { | ||
| case header, ok := <-headerCh: |
There was a problem hiding this comment.
There is an edge case possible here. The HeaderSub doesn't guarantee a contiguous stream of headers, and there might be gaps that we have to handle. It shouldn't be hard to handle those through the header store, though.
There was a problem hiding this comment.
I wasn't aware that it doesn't guarantee a contiguous stream of headers - to me, this sounds like something that needs to be fixed in headersub itself. Otherwise, we need to make it public because that is not intuitive and it's how everybody is currently using it. I would be fine holding off on merging this until that is fixed, can we discuss it further?
There was a problem hiding this comment.
What if we will cache non-contiguous headers?
There was a problem hiding this comment.
Agreed, we can move this issue separately. I think there might be a potential reason to fix this on go-header level with a Subscriber wrapper that does exactly gap filling. This way users relying on header subscriptions would not need to think of that edge case. Need to think of the implications of that tho
There was a problem hiding this comment.
Mind parking this in an issue?
blob/service.go
Outdated
| height uint64 | ||
| } | ||
|
|
||
| func (s *Service) Subscribe(ctx context.Context, ns share.Namespace) (<-chan *BlobsubResponse, error) { |
There was a problem hiding this comment.
These should follow GetAll semantics on the plurality of namespaces. We have yet to decide whether to keep GetAll with a single namespace or multiple; the subscription should be consistent with it, so we should decide on that first.
There was a problem hiding this comment.
(I would also call the method SubscribeAll)
There was a problem hiding this comment.
I don't think we should have multiple namespaces for subscription. For GetAll it's fine since it is a single call, but for a long-term subscription, it adds complexity in handling on the user's side. So, if you insist on consistency, I'd rather make GetAll with a single namespace than SubscribeAll with multiple namespaces.
There was a problem hiding this comment.
You would need to write two codepaths for the same blob handling logic, if these are inconsistent. As a dev, I would hate doing the same thing twice.
There was a problem hiding this comment.
On the single vs multiple namespace. Try to imagine how the implementation of the protocol that relies on multiple namespaces would look like(e.g. Rollkit). If you do it sequentially, you add another roundtrip. If you do it concurrently, you bring complexity for devs to synchronise both.
There was a problem hiding this comment.
For the reasons I stated in the description, I will not be extending this to add multiple namespaces. I also do not think consistency is so important here, these are two very different types of methods - and as I described, a single channel per namespace makes the most sense for multiple namespace handling anyways.
Changing GetAll is out of scope here and so I would defer that to a discussion. I don't think this feature should be blocked on this at all
There was a problem hiding this comment.
Try to imagine how the implementation of the protocol that relies on multiple namespaces
Having multiple subscriptions and a generic logic that parses them they will directly know from which namespace the blob comes.
Introduces
blob.Subscribe. Takes ashare.Namespace, returns a<-chan *BlobsubResponsecontaining aheight uint64andblobs []*blob.Blob.I made the decision to not support subscribing to multiple namespaces with one call, because the complexity of the return type mitigates the benefits for the client. It is likely more convenient to have one subscription per namespace:
Imagine you are watching two namespaces for different things. On the client side, you would have a
selectstatement reading from the two channels in two cases. This is cleaner than receiving everything in one case, but then having to manually parse and send to the respective handlers yourself.TODO: