runtime: fix five bugs #354
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jgraettinger
force-pushed
the
johnny/bug-fixes
branch
2 times, most recently
from
567395e
to
276b829
Compare
If StartCommit fails to transfer ownership of the futures to the client's readLoop. A failure to resolve this future _seems_ fine, but causes deadlock in certain conditions. Also tighten up some usage-related errors to be panics instead, to simplify the overall error flow. Also handle errors from publishing materialization stats. Fixes #352
Before, capture tasks spawned their own ShardSpec watch loops, and we were leaking these loops across connector invocations. Fix that (by only spawning new watch loops as required), but also centralize the creation of these watch loops and introduce a term context which is parented by the shard's context. Update shuffle.ReadBuilder to take a |doneCh|, which is simply the term's Context.Done(). Capture tasks also directly use the term context when starting up connector RPCs. Fixes #350
jgraettinger
force-pushed
the
johnny/bug-fixes
branch
from
276b829
to
e25bb05
Compare
jgraettinger
changed the title
jgraettinger
force-pushed
the
johnny/bug-fixes
branch
from
6548c2b
to
904fd86
Compare
To notify the server that it will no longer send acknowledgements. This isn't typically required with true gRPC streams, but it's good practice and it *does* cause a wedged go-routine with our in-process adapter. Issue #350
If a connector error occurrs, immediately surface it into the read channel (causing the shard to fail), rather than squelching it and waiting out the capture polling interval. The prior logic pre-dates the FAILED shard watchdog, and we now want to consolidate error handling to simply let the shard crash and allow the watchdog to sort out retries. Now, we'll only wait out a polling interval if the connector exits with EOF and the term context is not cancelled. This means that a new activation of the task will immediately re-start the polling interval, which is generally what the user wants & expects. Tested locally, by running both streaming and polled captures, manually un-assigning failed and non-failed shards, and coercing captures to fail with driver errors. Issue #350
|
FYI added two additional patches for new bugs discovered and discussed on #350 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits for fixes to #350 & #352. No behavior or documentation changes.
See also the related gazette fix: gazette/core#315
Testing:
This change is