KAFKA-12981 Ensure LogSegment.maxTimestampSoFar and LogSegment.offsetOfMaxTimestampSoFar are read/updated in sync#10960
Merged
dajac merged 10 commits intoapache:trunkfrom Jul 6, 2021
Conversation
…ent.maxTimestampSoFar to single tuple to ensure consistent update/read
dajac
reviewed
Jul 2, 2021
dajac
reviewed
Jul 5, 2021
Member
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
@thomaskwscott Thanks for the update. I left a few more minor comments.
Member
|
@thomaskwscott Could you also update the description of the PR? It would be great to explain how it relates to KIP-734 as well. |
ijuma
reviewed
Jul 5, 2021
ijuma
reviewed
Jul 5, 2021
| latestTimestampSegment.offsetOfMaxTimestampSoFar, | ||
| val latestTimestampAndOffset = latestTimestampSegment.maxTimestampAndOffsetSoFar | ||
| Some(new TimestampAndOffset(latestTimestampAndOffset.timestamp, | ||
| latestTimestampAndOffset.offset, |
Member
There was a problem hiding this comment.
Is this another instance of unnecessary copying? Are there others?
Member
There was a problem hiding this comment.
Oh, this is a different type (TimestampAndOffset).
Contributor
Author
There was a problem hiding this comment.
I had a check through and can't see any other copy instances
dajac
reviewed
Jul 6, 2021
Member
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
@thomaskwscott I left one small nit below.
|
|
||
| /* The maximum timestamp and offset we see so far */ | ||
| @volatile private var _maxTimestampAndOffsetSoFar: TimestampOffset = TimestampOffset.Unknown | ||
| def maxTimestampAndOffsetSoFar_= (timestampOffset: TimestampOffset) : Unit = _maxTimestampAndOffsetSoFar = timestampOffset |
Member
There was a problem hiding this comment.
nit: There is an extra space before : Unit.
Contributor
Author
There was a problem hiding this comment.
fixed this one and found a couple more.
lmr3796
pushed a commit
to lmr3796/kafka
that referenced
this pull request
Nov 19, 2021
TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - ~~Avoid appending to the time index during shutdown if the time index has not yet be initialized~~ This is covered in apache#8346 and apache#10960 EXIT_CRITERIA = TICKET [KAFKA-8667, KAFKA-8668] The patch is a sqaush of the 2 commits: == This is the 1st commit [155b4f8] == [LI-HOTFIX] Reduce lock retention and improve broker shutdown time: TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - Avoid appending to the time index during shutdown if the time index has not yet be initialized RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee == This is the commit #2 [c53fffd] == [LI-HOTFIX] Update fetcher thread idle flag in addPartitions TICKET = KAFKA-8667 LI_DESCRIPTION = This patch fixes in bug introduced by “[LI-HOTFIX] Reduce lock retention and improve broker shutdown time” HOTFIX where the fetcher thread idle flag is not set in addPartitions, which can cause idle fetcher thread not shutdown in time. RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee
xdgrulez
pushed a commit
to xdgrulez/kafka
that referenced
this pull request
Dec 22, 2021
…tOfMaxTimestampSoFar are read/updated in sync (apache#10960) This patch ensures that `maxTimestampSoFar` and `offsetOfMaxTimestampSoFar` are consistent with each others. It does so by storing them together. It relates to KIP-734 which exposes them via the admin client. Reviewers: Ismael Juma <ismael@juma.me.uk>, David Jacot <djacot@confluent.io>
lmr3796
pushed a commit
to linkedin/kafka
that referenced
this pull request
Jan 4, 2022
TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - ~~Avoid appending to the time index during shutdown if the time index has not yet be initialized~~ This is covered in apache#8346 and apache#10960 EXIT_CRITERIA = TICKET [KAFKA-8667, KAFKA-8668] The patch is a sqaush of the 2 commits: == This is the 1st commit [155b4f8] == [LI-HOTFIX] Reduce lock retention and improve broker shutdown time: TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - Avoid appending to the time index during shutdown if the time index has not yet be initialized RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee == This is the commit #2 [c53fffd] == [LI-HOTFIX] Update fetcher thread idle flag in addPartitions TICKET = KAFKA-8667 LI_DESCRIPTION = This patch fixes in bug introduced by “[LI-HOTFIX] Reduce lock retention and improve broker shutdown time” HOTFIX where the fetcher thread idle flag is not set in addPartitions, which can cause idle fetcher thread not shutdown in time. RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee
lmr3796
pushed a commit
to lmr3796/kafka
that referenced
this pull request
Feb 9, 2022
TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - ~~Avoid appending to the time index during shutdown if the time index has not yet be initialized~~ This is covered in apache#8346 and apache#10960 EXIT_CRITERIA = TICKET [KAFKA-8667, KAFKA-8668] The patch is a sqaush of the 2 commits: == This is the 1st commit [155b4f8] == [LI-HOTFIX] Reduce lock retention and improve broker shutdown time: TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - Avoid appending to the time index during shutdown if the time index has not yet be initialized RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee == This is the commit #2 [c53fffd] == [LI-HOTFIX] Update fetcher thread idle flag in addPartitions TICKET = KAFKA-8667 LI_DESCRIPTION = This patch fixes in bug introduced by “[LI-HOTFIX] Reduce lock retention and improve broker shutdown time” HOTFIX where the fetcher thread idle flag is not set in addPartitions, which can cause idle fetcher thread not shutdown in time. RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee
lmr3796
pushed a commit
to lmr3796/kafka
that referenced
this pull request
Mar 25, 2022
TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - ~~Avoid appending to the time index during shutdown if the time index has not yet be initialized~~ This is covered in apache#8346 and apache#10960 EXIT_CRITERIA = TICKET [KAFKA-8667, KAFKA-8668] The patch is a sqaush of the 2 commits: == This is the 1st commit [155b4f8] == [LI-HOTFIX] Reduce lock retention and improve broker shutdown time: TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - Avoid appending to the time index during shutdown if the time index has not yet be initialized RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee == This is the commit #2 [c53fffd] == [LI-HOTFIX] Update fetcher thread idle flag in addPartitions TICKET = KAFKA-8667 LI_DESCRIPTION = This patch fixes in bug introduced by “[LI-HOTFIX] Reduce lock retention and improve broker shutdown time” HOTFIX where the fetcher thread idle flag is not set in addPartitions, which can cause idle fetcher thread not shutdown in time. RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee
lmr3796
pushed a commit
to lmr3796/kafka
that referenced
this pull request
Jun 2, 2022
TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - ~~Avoid appending to the time index during shutdown if the time index has not yet be initialized~~ This is covered in apache#8346 and apache#10960 EXIT_CRITERIA = TICKET [KAFKA-8667, KAFKA-8668] The patch is a sqaush of the 2 commits: == This is the 1st commit [155b4f8] == [LI-HOTFIX] Reduce lock retention and improve broker shutdown time: TICKET = [KAFKA-8667, KAFKA-8668] LI_DESCRIPTION = - Avoid acquiring partitionMap lock in shutdownIdleFetcherThread - Avoid appending to the time index during shutdown if the time index has not yet be initialized RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee == This is the commit #2 [c53fffd] == [LI-HOTFIX] Update fetcher thread idle flag in addPartitions TICKET = KAFKA-8667 LI_DESCRIPTION = This patch fixes in bug introduced by “[LI-HOTFIX] Reduce lock retention and improve broker shutdown time” HOTFIX where the fetcher thread idle flag is not set in addPartitions, which can cause idle fetcher thread not shutdown in time. RB=1431408 BUG=LIKAFKA-19361 G=Kafka-Code-Reviews R=jkoshy,jonlee A=jkoshy,jonlee
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
3 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.
This PR refactors LogSegment.offsetOfMaxTimestampSoFar and LogSegment.maxTimestampSoFar to single tuple to ensure consistent update/read
This ties in with: https://cwiki.apache.org/confluence/display/KAFKA/KIP-734%3A+Improve+AdminClient.listOffsets+to+return+timestamp+and+offset+for+the+record+with+the+largest+timestamp
as we can now use the adminClient to fetch timestamps/offsets with the largest timestamp in a partition. This refactor avoids a possible race condition where one of timestamp/offset us updated before the other is read (e.g. the dminclinet fetches the highest timestamp and then the offset changes before it is read)
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)