Fix retry logic in BrokerClient and flakey BrokerClientTest
#16618
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 testError() method reliably fails in the IDE. This is because the the test runner has <surefire.rerunFailingTestsCount>3</surefire.rerunFailingTestsCount> is set to 3, so maven retries this "flaky test" multiple times and the test code returns a successful response in the third attempt. The exception handling in BrokerClientTest was broken: - All non-2xx errors were being turned as 5xx errors. Remove that block of code. If we need to handle retries of more specific 5xx error codes, that should be hanlded explicitly. Or if there's a source of 4xx class error that needs to be 5xx, fix that in the source of error.
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
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.
BrokerClientTestwas added in PR #14322. When running this test from the IDE (outside of Maven),testError()reliably fails. However, it passes in CI due to the Maven setting<surefire.rerunFailingTestsCount>3</surefire.rerunFailingTestsCount>in the pom.xml. This setting allows Maven to retry the test up to three times, and the test ultimately passes because of the retry logic in the code, but maven treats it as "flaky".Changes
Looking into the error handling in
BrokerClient, it was found that the retry logic doesn't effectively handle 5xxDruidExceptioncategories since the HTTP error codes are converted intoDruidExceptions. So the retry logic didn't fully account transient errors and this patch fixes that. We could perhaps change the semantics of the method to just use the HTTP response, or write a service client that uses the built-in mechanisms, but it'll be a larger change.Also, I removed the code that converts all non-200 error codes into retryable 5xx DruidException categories. For example, 4xx error codes shouldn't be retried. If there are additional 500 error codes we need to retry, we can add them explicitly similar to the ones that are currently handled.
This PR has: