KAFKA-17166: Use NoOpScheduler to rewrite LogManagerTest#testLogRecoveryMetrics#16641
KAFKA-17166: Use NoOpScheduler to rewrite LogManagerTest#testLogRecoveryMetrics#16641chia7712 merged 1 commit intoapache:trunkfrom
Conversation
| appendRecordsToLog(mockTime, logDir1, 0, mockBrokerTopicStats, expectedSegmentsPerLog) | ||
| appendRecordsToLog(mockTime, logDir2, 1, mockBrokerTopicStats, expectedSegmentsPerLog) | ||
|
|
||
| val noOpsScheduler = new NoOpsScheduler() |
There was a problem hiding this comment.
How about creating a (noop) mock scheduler? that is more simple :)
There was a problem hiding this comment.
Yes, using mock can have same effect. Thank you.
FrankYang0529
force-pushed
the
KAFKA-17166
branch
2 times, most recently
from
dc3093c to
0c1c1c0
Compare
ocadaruma
left a comment
ocadaruma
left a comment
There was a problem hiding this comment.
Thank you for your quick fix.
LGTM
|
@FrankYang0529 Could you please rebase code to trigger QA again? |
…eryMetrics Signed-off-by: PoAn Yang <payang@apache.org>
FrankYang0529
force-pushed
the
KAFKA-17166
branch
from
0c1c1c0 to
a7a1e6b
Compare
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@FrankYang0529 : Thanks for the PR. Just a couple of minor comments.
| } | ||
| } | ||
|
|
||
| private void writeToFileForTruncation() { |
There was a problem hiding this comment.
Perhaps it's clearer to rename this and checkpoint.writeForTruncation to sth like writeIfDirExists.
| private void writeToFileForTruncation() { | ||
| lock.readLock().lock(); | ||
| try { | ||
| checkpoint.writeForTruncation(epochs.values()); |
There was a problem hiding this comment.
Could we add a comment on why it's important to hold the lock while writing the checkpoint file?
|
Also, could we cherry-pick this PR to 3.8 since it has the same correctness issue? |
That will make 3.8 get in next RC. @jlprat WDYT? |
|
Would this be considered a blocker for the release? We can definitely cherry pick it on the |
This is correctness issue, so it should be considered a blocker even though it should be rare. I will cherry-pick it (and #16614) to 3.8 later |
|
Hi @chia7712 could you share this in the Vote thread for RC2? Just for completion / traceability https://lists.apache.org/thread/ownfopb4rmdkdqtqwcnzl3x6lbr03ql8 |
done |
…ryMetrics (#16641) Reviewers: Okada Haruki <ocadaruma@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
done. I have updated the jiras also. @FrankYang0529 Please file a MINOR PR to address @junrao if you have free cycles. |
Yes, I will do it today. |
…ryMetrics (apache#16641) Reviewers: Okada Haruki <ocadaruma@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
5 participants
LogManagerTest#testLogRecoveryMetricsdoes not require the scheduler, so we can introduce theNoOpSchedulerto handle the test case.checkpoint.writeForTruncation(epochs.values());to underLeaderEpochFileCacheread lock to avoid race condition like:Committer Checklist (excluded from commit message)