streamingccl: fix producer stream to properly propagate error #85867
Conversation
|
This change is |
There was a problem hiding this comment.
Hi folks. This is a bug I found when I tried to see what happens if the source cluster issues a non-MVCC op like ClearRange, the behavior I observed is that the producer stream just hang there, instead of erroring out to make a subscription to exit.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @miretskiy, @samiskin, and @stevendanna)
| @@ -50,6 +50,7 @@ type eventStream struct { | |||
| // Fields below initialized when Start called. | |||
| rf *rangefeed.RangeFeed // Currently running rangefeed. | |||
| streamGroup ctxgroup.Group // Context group controlling stream execution. | |||
| closeChan chan struct{} // Channel signaled to close the stream loop. | |||
There was a problem hiding this comment.
I think it would be nice to keep 1 channel.
If I understood the problem you're describing, then closeChan
could be renamed to doneChan. errCh can be removed. When error happens (maybeSetError), close doneChan, and save the terminal error.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/streamingccl/streamproducer/event_stream.go line 53 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
I think it would be nice to keep 1 channel.
If I understood the problem you're describing, then closeChan
could be renamed to doneChan. errCh can be removed. When error happens (maybeSetError), close doneChan, and save the terminal error.
I renamed the closeCh to doneCh. Although it sounds nice and simple, removing errCh actually is making it very hard to implement it right because
- there are two callsites where we want to close this doneChan: rangefeed internalError callback and the eventStream.Close. But channel cannot be closed twice.
- Having a doneChan and err make it hard to satisfy following usage:
- rangefeed internalErr callback sets err and close
doneCh - the Next will need to react for
doneChand then readserrand it needs to know other errors occurred in streamLoop. - so streamLoop needs to set
errwhen other errors occur, that will cause race condition.
So using another channel doneCh is probably the easiest and cleaner way to do it.
gh-casper
force-pushed
the
fix-cursor-err
branch
from
1edb0ac to
f89820c
Compare
gh-casper
force-pushed
the
fix-cursor-err
branch
from
f89820c to
078a0af
Compare
There was a problem hiding this comment.
I imagine this is also why we currently just see the entire stream hang when you don't have rangefeeds enabled on the source side.
Just to make sure I understand your commit message and the code:
The bug is currently that when we get a rangefeed error, we push the error into the errCh. The SQL receive sees this error, but expect to be able to Close() the generator before returning it. Our Close() function would hang forever since nothing was going to end up shutting it down.
There was a problem hiding this comment.
Exactly.
Reviewable status:
Previously when rangefeed returns internal error, the error didn't get propagated through the producer stream cursor. The bug is that the SQL receiver's result writer only send error after all ValueGenerator close. However, closing ventStream (i.e., the producer stream ValueGenerator) relies on shutting down streamLoop which is not explicitly shut down during Close. Release justification: bug fixes and low-risk updates to new functionality Release note: None
gh-casper
force-pushed
the
fix-cursor-err
branch
from
078a0af to
c68231c
Compare
|
bors r+ |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
Previously when rangefeed returns internal error, the error
didn't get propagated through the producer stream cursor.
The bug is that the SQL receiver's result writer only sends
error after all ValueGenerators close. However, closing eventStream
(i.e., the producer stream ValueGenerator) relies on shutting
down of streamLoop which is not explicitly shut down during Close.
Release justification: bug fixes and low-risk updates to
new functionality
Release note: None