MINOR: add comment for correctness issue to LeaderEpochFileCache#16660
Conversation
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@FrankYang0529 : Thanks for the PR. Just a minor comment.
| checkpoint.writeForTruncation(epochs.values()); | ||
| // Write epoch entries under the read lock to avoid older epoch entries overwriting the newer epoch entries. | ||
| // If we take a snapshot of the epoch entries here and flush to disk outside the read lock, | ||
| // the leader epoch file may have correctness issue, because the new epoch entry may already in the file. |
There was a problem hiding this comment.
Perhaps rewording to sth like the following?
If we take a snapshot of the epoch entries here and flush them to disk outside the read lock, by the time of flushing, the leader epoch file may already be updated with newer epoch entries. Those newer entries will then be overridden with the old snapshot.
There was a problem hiding this comment.
Thanks for the review and suggestion. Updated it.
Signed-off-by: PoAn Yang <payang@apache.org>
FrankYang0529
force-pushed
the
minor-add-comment-to-leader-epoch-file-cache
branch
from
910da0f to
584b5e7
Compare
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@FrankYang0529 : Thanks for the updated PR. LGTM
|
JDK 11, JDK 17 and JDK 21 pass. I run JDK8 on my local and it works well. Will merge it. |
|
I re-trigger the QA again. It seems JDK8 gets hanging. not sure whether that is related to #16641. will loop tests on my local at the mean time |
I don't reproduce the hanging on my local. Will merge this PR and then keep my eyes on it ... |
…che#16660) Reviewers: Jun Rao <junrao@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
FrankYang0529
restored the
minor-add-comment-to-leader-epoch-file-cache
branch
3 participants
Follow up: #16641
LeaderEpochCheckpointFile#writeForTruncationtoLeaderEpochCheckpointFile#writeIfDirExists.LeaderEpochFileCache#writeToFileForTruncationtoLeaderEpochFileCache#writeIfDirExists.Committer Checklist (excluded from commit message)