Default request timeout in cluster #739
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
The cluster needs to know the request timeout to know how long it should hold the target topic mutation lock. Fixes #695.
The intention is to make it easier to target flaky tests for fixing, by running `yarn test:local:watch --testNamePattern=flaky`.
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.
This moves the default request timeout up from the connection up to the cluster, as with the connection timeout. The target topic mutation lock is using the request timeout as a proxy for how long it should wait before releasing the lock. With the current code, if the cluster doesn't have a request timeout set, the lock gets the default connection timeout of 1000ms, while the request itself doesn't time out until 29 seconds later.
In doing this, I also removed the default timeout from the Lock, as the default wasn't actually being used anywhere, and I didn't think a default made any sense really, since it depends entirely on what the lock is being used for.
This hopefully fixes #695.
I also added a way to mark tests as flaky, as I noticed that one of the tests in
assignmentForUnsubscribedTopicfailed every 10-20 runs or so on my local machine, and while I didn't have the energy to try and fix it right now, I think it makes sense to mark it as flaky so that it's easier to know in the future where to direct your efforts. All it does is prefix the description of the test with[flaky]so that you can target flaky tests directly using--testNamePattern=flaky.