showuon commented on PR #14242: URL: https://github.com/apache/kafka/pull/14242#issuecomment-1704740274
@ocadaruma , thanks for the improvement! Some high level questions: 0. Although you've added comments in the JIRA, it'd better you add your analysis and what/why you've changed in the PR description. 1. moving fsync call to the scheduler thread in `takeSnapshot` case makes sense to me, since we have every info in memory cache. And the log recovery can recover the snapshot when unclean shutdown. 2. For `removeAndMarkSnapshotForDeletion`, I didn't see this fix, could you explain it? 3. For `LeaderEpochFileCache#truncateXXX`, I agree that as long as the memory cache is up-to-date, it should be fine. We can always recover from logs when unclean shutdown. 4. nit: In the PR, could we make less code change for easier review? That is, we can create a overloaded method, and take one more parameter (boolean sync), and delegate the original method implementation to the new method (default to true). So, the only place we need to change, is the places we want to `false` the sync flush, which will make the PR much clear IMO. Thank you. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
