-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optimistic channels #341
Add optimistic channels #341
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is should these ObjectStore
or S3Store
. I'm torn because I don't want us to be wed to a vendor in the implementation on the one hand, but on the other hand the other object stores in the market tend to implement the S3 API because they are a leader. My hesitation feels a little nit picky and pedantic and can probably be deferred.
Explicitly keeping S3 in the codebase as a store type is something we probably want to move away from.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @allada)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @aaronmondal)
util/buf_channel.rs
line 58 at r1 (raw file):
/// the receiver can be trusted to handle failing streams this is safe to do. #[must_use] pub fn make_buf_channel_pair_optimistic_writer() -> (DropCloserWriteHalf, DropCloserReadHalf) {
Why not just add .disable_eof_check(&mut self)
on the writer?
util/buf_channel.rs
line 88 at r1 (raw file):
/// receiver end's drop status (ie: if the receiver dropped unexpectedly). close_rx: oneshot::Receiver<Result<(), Error>>, disable_eof_check: bool,
It looks like we already have close_after_size
on DropCloserReadHalf
. I think we can use this flag to ensure the read half can get into this configuration.
As for the Writer, would it make more sense to simply add a similar kind of flag check it insend_eof()
and not emit an error if the flag is set (ie: set_ignore_eof()
)?
util/buf_channel.rs
line 131 at r1 (raw file):
{ loop { if let Some(maybe_chunk) = reader.next().await {
nit: I personally prefer the match
syntax here, it feels easier to read to me. but just my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @aaronmondal)
Work around the fact that S3 drops connections as soon as all data is sent or received. Fixes TraceMachina#304 and fixes the same issue for non-oneshot cases where a similar issue occurred as well.
d8bf8c0
to
2137225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @aaronmondal and @allada)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusSorealheis I agree that we don't want to name this S3. Something like ObjectStore
or BlobStore
seems more appropriate and lets us use more vendor-specific libraries to interact with AWS, GCP, Azure and custom solutions like CubeFS, Ceph and Minio.
I'll soon have a new implementation for S3 via the AWS SDK instead of rusoto ready. I still need to look into others. Then we can generalize the current implementation.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @allada)
util/buf_channel.rs
line 58 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Why not just add
.disable_eof_check(&mut self)
on the writer?
Done. Yes that's much better 👍
util/buf_channel.rs
line 88 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
It looks like we already have
close_after_size
onDropCloserReadHalf
. I think we can use this flag to ensure the read half can get into this configuration.As for the Writer, would it make more sense to simply add a similar kind of flag check it in
send_eof()
and not emit an error if the flag is set (ie:set_ignore_eof()
)?
Not sure I understood you correctly here. Is the new version what you envision?
util/buf_channel.rs
line 131 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: I personally prefer the
match
syntax here, it feels easier to read to me. but just my opinion.
That was flagged by clippy::single_match_else I'd prefer the new variant since it reduces indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aaronmondal)
util/buf_channel.rs
line 88 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
Not sure I understood you correctly here. Is the new version what you envision?
Yep, this is exactly what I was referring :-) ... To me this is so much easier to read.
Work around the fact that S3 drops connections as soon as all data is sent or received. Fixes TraceMachina#304 and fixes the same issue for non-oneshot cases where a similar issue occurred as well.
Work around the fact that S3 drops connections as soon as all data is sent or received. Fixes TraceMachina#304 and fixes the same issue for non-oneshot cases where a similar issue occurred as well.
This variation on make_buf_channel_pair omits the EOF check. This works around the fact that S3 drops connections as soon as all data is sent or received.
Fixes #304 and fixes the same issue for non-oneshot cases where a similar issue occurred as well.
This change is