Alexey Serbin 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: (4 comments) http://gerrit.cloudera.org:8080/#/c/24963/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24963/1//COMMIT_MSG@10 PS1, Line 10: Writrable > typo: Writable Done http://gerrit.cloudera.org:8080/#/c/24963/1//COMMIT_MSG@17 PS1, Line 17: EXPECT_FALSE > PREDICT_FALSE ? Done 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 Right -- all those syscalls are about synchronizing the contents of the file with its run-time image, and they are simply a set of flavors of doing that for a particular use-case. There isn't much difference categorically. It could be a good idea to update the name of the flag, but I'd rather do so in a separate changelist, indeed. Renaming the flag shouldn't be a big deal from the backwards compatibility perspective since it's a hidden/unsafe flag which is nobody should be using in real production systems. Also, I'd prefer something a bit narrower, e.g. never_fs_sync or no_explicit_fs_sync. 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 F Right -- there isn't much coverage for this stuff since the original code came from LevelDB -- it's similar to other utilities imported into the Kudu's codebase. Yes, I can add test coverage based on trace counters -- that's quite fragile and don't test actual functionality (i.e. checking whether there was a call to fsync(), or whether it was fdatasync(), but it could help in checking out the control flow paths. Adding such tests satisfies formal criteria of 'having test coverage', but in general I'd think of better ways of testing this from the perspective of actual functionality. However, ad-hoc approach with trace counters/points should be good enough as a first step, indeed. -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 30 Sep 2026 18:46:28 +0000 Gerrit-HasComments: Yes
