-
Notifications
You must be signed in to change notification settings - Fork 1.5k
doc: update RecordBatchReceiverStreamBuilder::spawn_blocking task behaviour #14995
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
Conversation
422e719
to
25ee148
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.
Thank you @shruti2522 🙏
cc @tv42 perhaps you can review this PR as well?
/// built from it) are dropped | ||
/// Spawns a blocking task that attempts to send `RecordBatch` data to this stream. | ||
/// | ||
/// If this builder (or the stream built from it) is dropped **before** the task starts, |
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.
I think here can be some confusion. Task will never executes does that mean the task ignored or it is stuck in waiting queue forever?
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.
I think "task will never execute" means it is droped and never runs.
@@ -241,8 +241,17 @@ impl RecordBatchReceiverStreamBuilder { | |||
self.inner.spawn(task) | |||
} | |||
|
|||
/// Spawn a blocking task that will be aborted if this builder (or the stream | |||
/// built from it) are dropped | |||
/// Spawns a blocking task that attempts to send `RecordBatch` data to this stream. |
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.
Current usages can be limited with this use case, but if we look at the signature, this util is not limited with that case only. Perhaps we can generalize this more?
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.
Maybe we can restore the original description as I think that was more accurate
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.
Thanks @berkaysynnada and @comphead and @shruti2522
I took the liberty of pushing a commit that I think clarifies the documentation. Let me know what you think!
/// # Drop / Cancel Behavior | ||
/// | ||
/// If this builder (or the stream built from it) is dropped **before** the | ||
/// task starts, the task is also dropped and will never start execute. |
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.
👍
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.
lgtm thanks @alamb and @shruti2522
Thanks again @shruti2522 and @comphead |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
update documentation for
RecordBatchReceiverStreamBuilder::spawn_blocking
Are these changes tested?
Are there any user-facing changes?