KAFKA-12879: Revert changes from KAFKA-12339 and instead add retry capability to KafkaBasedLog#11797
Merged
Merged
Conversation
rhauch
changed the title
rhauch
reviewed
Feb 24, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @philipnee. I have a few comments below, but otherwise this PR is pretty close.
rhauch
reviewed
Feb 25, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @philipnee. I have a few more minor comments below.
- javaDoc - better logging
rhauch
reviewed
Feb 25, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Thanks again, @philipnee. Getting to very nitty details now, but I think if all of these are addressed then we should be good.
- Updated documentation - Updated log message in the retry util, now getting ```[2022-02-25 13:18:34,313] WARN RetriableException caught on attempt 1, retrying automatically up to 9 more times. Reason: timeout exception (org.apache.kafka.connect.util.RetryUtil:62) org.apache.kafka.common.errors.TimeoutException: timeout exception``` - Improved RetryUtil logic: no retry if maxRetries = 0, no extra sleep for the last retry attempt - lower case for test methods
rhauch
reviewed
Feb 25, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Okay, just a few more nits related to your recent changes.
- minors
…tryUtil.java accepting comments on spacing Co-authored-by: Randall Hauch <rhauch@gmail.com>
rhauch
reviewed
Mar 2, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @philipnee. A few more nits below, but one bigger suggestion around how we know how long to retry. I know I originally suggested the approach in this PR, but I do think my suggested changes are going to be much better.
Co-authored-by: Randall Hauch <rhauch@gmail.com>
rhauch
reviewed
Mar 2, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Very nice, @philipnee! A few nits below, but otherwise looks good.
Co-authored-by: Randall Hauch <rhauch@gmail.com>
rhauch
reviewed
Mar 3, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the update, @philipnee. I have some very nit-picky suggestions around logging and error messages, which IMO are important to get right so that Connect users know very clearly what's happened when looking at logs or errors.
I also found one potential logic bug in the edge case when the remaining time to retry is smaller than the retry backoff time.
I think if we fix these and the build passes, we're good!
Co-authored-by: Randall Hauch <rhauch@gmail.com>
- documentation
clean up the sleep logic, add tests, and logging
rhauch
approved these changes
Mar 7, 2022
rhauch
left a comment
rhauch
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @philipnee! LGTM.
@ijuma, do you want to double-check the reversion of KAFKA-12339 / #10152?
3 tasks
3 tasks
3 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes the compatibility issue regarding KAFKA-12879 by reverting the changes to the admin client from KAFKA-12339 (#10152), and instead perform the retries within the KafkaBasedLog via a new
TopicAdmin.retryEndOffsets(..). This method will delegate to the existingTopicAdmin.endOffsets(...)and will retry on RetriableException until the retries (max retries) is hit, or until a specified timeout.This change should be backward compatible to the KAFKA-12339.
Notable changes are:
Reverted KAFKA-12339
TopicAdmin
Added new
retryEndOffsets(...)method that utilizes the newRetryUtilto list end offsets for a set of topic partitions and perform retries upon any RetriableException (including UnknownTopicOrPartitionException, TimeoutException).KafkaBasdLog
During startup calls
TopicAdmin.retryEndOffsets(...)to read end offsets for the log topic and retries if the topic is not available (or any RetriableException).RetryUtil
A general utility that takes a Callable function, the total duration to retry (may be 0), and backoff time in ms, and that calls the function at least once, and performs retries using the parameters when the function throws RetriableException (e.g. UnknownTopicOrPartitionException, TimeoutException)
Committer Checklist (excluded from commit message)