[GitHub] [kafka] showuon commented on pull request #14242: KAFKA-15046: Get rid of unnecessary fsyncs inside UnifiedLog.lock to stabilize performance

["showuon (via GitHub)" <gi...@apache.org>]

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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org