Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/24963 )
Change subject: [util] an update on --never_fsync flag usage ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/24963/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24963/1//COMMIT_MSG@17 PS1, Line 17: EXPECT_FALSE PREDICT_FALSE ? http://gerrit.cloudera.org:8080/#/c/24963/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/24963/1/src/kudu/util/env_posix.cc@182 PS1, Line 182: DEFINE_bool(never_fsync, false, : "Never fsync() anything to disk. This is used by certain test cases to " : "speed up runtime. This is very unsafe to use in production."); The flag name 'never_fsync' can be quite confusing because it specifically mentions fsync and leaves out the detail that it includes fsync(), fdatasync(), sync_file_range(). In other words, if 'never_fsync' is set to true, calls like DoSync(), Flush() and SyncDir() would exit early without calling fsync() or fdatasync() or sync_file_range(). So, essentially this should be something like 'never_sync'. Changing the flag name along with the description would be the right course here but that expands the scope of this change, so an elaborate description for now should be worth doing. What do you think? http://gerrit.cloudera.org:8080/#/c/24963/1/src/kudu/util/env_posix.cc@437 PS1, Line 437: DoSync There seems to be limited existing test coverage for this path as well as Flush(), SyncDir() w.r.t. FLAGS_never_fsync and FLAGS_env_use_fsync values. Maybe add a couple unit tests in env-test.cc and make use of existing trace counters (fsync, fdatasync, etc) and add new one for 'sync_file_range' to test that appropriate paths are taken in accordance with the flag values. Thoughts? -- To view, visit http://gerrit.cloudera.org:8080/24963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I637e3bd98da98495a38f78176a4a862ab6ae8dd3 Gerrit-Change-Number: 24963 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 28 Sep 2026 07:03:31 +0000 Gerrit-HasComments: Yes
