kvcoord: Correctly handle stuck rangefeeds #92582
Conversation
|
This change is |
| @@ -71,11 +72,11 @@ func (w *stuckRangeFeedCanceler) stop() { | |||
|
|
|||
| // ping notifies the canceler that the rangefeed has received an | |||
| // event, i.e. is making progress. | |||
| func (w *stuckRangeFeedCanceler) ping() { | |||
| func (w *stuckRangeFeedCanceler) do(fn func() error) error { | |||
There was a problem hiding this comment.
The way this is currently structured, we don't need to return error from either of these functions. The closure can close over err and we can check it outside of do().
There are a couple of other approaches we can consider:
-
Have
do()also do error handling, returningerrRestartStuckRangewhen the rangefeed is stuck. -
Return
trueif the rangefeed is stuck when the function returns.
These would allow us to get rid of the stuck() method and simplify the caller logic. I think I have a preference for returning errRestartStuckRange and encapsulating as much of the logic as possible inside do(), but it's not a strongly held view. The current approach is also fine.
There was a problem hiding this comment.
Ack -- I was thinking of doing that. Still, I think returning an error is better -- I prefer not to close over errors.
There was a problem hiding this comment.
Not sure I can get rid of stuck() -- we call it in few places , and I think it just reads better when error handling is explicit.
There was a problem hiding this comment.
Ok. I'm not sure why we bother with returning errStuckRangefeed from do() in that case, since none of the callers use it anyway. You call though, nbd.
There was a problem hiding this comment.
Reverted return of errStuckRange.
| @@ -71,11 +72,11 @@ func (w *stuckRangeFeedCanceler) stop() { | |||
|
|
|||
| // ping notifies the canceler that the rangefeed has received an | |||
| // event, i.e. is making progress. | |||
| func (w *stuckRangeFeedCanceler) ping() { | |||
| func (w *stuckRangeFeedCanceler) do(fn func() error) error { | |||
There was a problem hiding this comment.
Ok. I'm not sure why we bother with returning errStuckRangefeed from do() in that case, since none of the callers use it anyway. You call though, nbd.
Added canceller test. |
Fixes cockroachdb#92570 A watcher responsible for restarting stuck range feeds may incorrectly cancel rangefeed if the downstream event consumer is slow. Release note (bug fix): Fix incorrect cancellation logic when attempting to detect stuck range feeds.
|
bors r+ |
1 similar comment
|
bors r+ |
|
Already running a review |
|
Build succeeded: |
Fixes #92570
A watcher responsible for restarting stuck range feeds may incorrectly cancel rangefeed if the downstream event consumer is slow.
Release note (bug fix): Fix incorrect cancellation logic when attempting to detect stuck range feeds.