ocadaruma commented on code in PR #16614:
URL: https://github.com/apache/kafka/pull/16614#discussion_r1683644380
##########
storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java:
##########
@@ -348,7 +348,8 @@ public void truncateFromEndAsyncFlush(long endOffset) {
// - We still flush the change in #assign synchronously,
meaning that it's guaranteed that the checkpoint file always has no missing
entries.
// * Even when stale epochs are restored from the checkpoint
file after the unclean shutdown, it will be handled by
// another truncateFromEnd call on log loading procedure,
so it won't be a problem
- scheduler.scheduleOnce("leader-epoch-cache-flush-" +
topicPartition, this::writeToFileForTruncation);
+ List<EpochEntry> entries = new ArrayList<>(epochs.values());
+ scheduler.scheduleOnce("leader-epoch-cache-flush-" +
topicPartition, () -> checkpoint.writeForTruncation(entries));
Review Comment:
@chia7712 Hi, thank you for pointing out the potential race issue exists
even on current code.
The follow-up looks good to me.
For follow-up 2 which moves checkpoint-flush to inside the lock, one concern
is potential request-handler/replica-fetcher thread blocking due to the fsync
latency. (i.e. threads call truncateFromStart/EndAsyncFlush will be blocked
meanwhile)
However it might not be the critical performance issue because:
- These methods are not called frequently (typical call path is truncation
during fetch response handling and deleteRecords handling), so it will unlikely
be called when writeToFileForTruncation (scheduled by previous method call) is
ongoing and causes lock contention.
* Unless kafka-schedulers are very busy and task execution is delayed
Let me consider some optimization is possible for this as an another
follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
