Skip to content

tcp_boundary: drop source correctly#11987

Closed
antiguru wants to merge 1 commit into
MaterializeInc:mainfrom
antiguru:token_drop
Closed

tcp_boundary: drop source correctly#11987
antiguru wants to merge 1 commit into
MaterializeInc:mainfrom
antiguru:token_drop

Conversation

@antiguru

Copy link
Copy Markdown
Member

Instead of waiting to schedule ok and err streams to drop the source, only
require a single of the streams to be terminate to terminate both. This
prevents a situation where the ok stream is terminated, but the err stream
will not pick up the termination because it won't be scheduled anymore.

Signed-off-by: Moritz Hoffmann mh@materialize.com

Motivation

  • This PR fixes a previously unreported bug: Sources aren't cleaned up after compute.

Testing

  • This PR has adequate test coverage / QA involvement has been duly considered.

Release notes

This PR includes the following user-facing behavior changes:

  • N/A

Instead of waiting to schedule ok and err streams to drop the source, only
require a single of the streams to be terminate to terminate both. This
prevents a situation where the ok stream is terminated, but the err stream
will not pick up the termination because it won't be scheduled anymore.

Signed-off-by: Moritz Hoffmann <mh@materialize.com>
@antiguru

Copy link
Copy Markdown
Member Author

This is an alternative to #11945.

@guswynn guswynn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am convinced now that #11945 is definitely a backwards approach, but this technique concerns me for 2 reasons:

  • What is different about the ok and err stream where, as the source is dying? Is the ok stream always going to get a push? if it is also not getting any data (just like the err) stream, could neither of these be called and we have the same bug?
  • My other comment inline; I don't understand timely well enough to know if this is a concern

Comment thread src/storage/src/boundary/tcp_boundary.rs
@guswynn

guswynn commented Apr 27, 2022

Copy link
Copy Markdown
Contributor

#12082 should be able to replace this pr

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@frankmcsherry

Copy link
Copy Markdown
Contributor

On my list of "pending reviews" but I suspect we just want to close?

@guswynn

guswynn commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

@frankmcsherry yeah, this should be relevant anymore!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants