Initial DA service sketch #376
Conversation
Codecov ReportPatch has no changes to coverable lines. ❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.
📢 Thoughts on this report? Let us know!. |
|
|
||
| async fn blob_stream( | ||
| &self, | ||
| ) -> Box<dyn Stream<Item = (Self::Blob, oneshot::Sender<Self::Reply>)> + Unpin + Send>; |
There was a problem hiding this comment.
mmm, thinking how the reply would work
Is your idea that the adapter somehow knows this and can create that channel on its own?
In that case, I think it should also be responsible for sending the blob, or better, we should unify the responsibilities of sending and interpreting the same message
There was a problem hiding this comment.
? Is your idea that the adapter somehow knows this and can create that channel on its own?
yes, this was just the first idea. It is probably gonna change. As for the first version we would be broadcasting the replies anyway.
I think actually for now Im just gonna remove this for what I explained above. And add a call to send the reply.
There was a problem hiding this comment.
Is your idea that the adapter somehow knows this and can create that channel on its own?
Yes, or at least there's a single component that knows how to read and write those.
What I want to avoid is that we define reading in one component and writing in another one, because we've know created hidden coupling between them
There was a problem hiding this comment.
Absolutely right. And the network adapter should not know about it.
|
|
||
| fn add_blob(&mut self, blob: Self::Blob) -> Result<(), DaError>; | ||
|
|
||
| fn pending_blobs(&self) -> Box<dyn Iterator<Item = Self::Blob> + Send>; |
There was a problem hiding this comment.
the expectation is that pending blobs are removed from the list once returned? or once polled from the iterator? or is that signaled externally?
There was a problem hiding this comment.
The idea is to remove the blobs in the same fashion we do in the mempool actually. We can use some evicting policy for really old ones as well.
There was a problem hiding this comment.
I don't see any method to change the blobs status though here
There was a problem hiding this comment.
Yes. Ill add it in the incoming backend PR.
Notice that traits and/or bounds may be changed. This is an initial approach and things would have to change probably a bit when piping everything together. I'll be implementing the backend and adapters after this.