Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19915 )
Change subject: [ut] Add '--log_cleanup_interval_sec' flag and speedup an unit test ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/19915/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19915/3//COMMIT_MSG@7 PS3, Line 7: ut server http://gerrit.cloudera.org:8080/#/c/19915/3//COMMIT_MSG@7 PS3, Line 7: an a http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc File src/kudu/integration-tests/log-rolling-itest.cc: http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@56 PS3, Line 56: TEST_F(LogRollingITest, TestLogCleanupOnStartup) { Since you are modifying this test in this patch, consider adding SKIP_IF_SLOW_NOT_ALLOWED() in the very beginning of the test since even with this patch the test takes about one minute (?) to complete. http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@64 PS3, Line 64: enables enable http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@64 PS3, Line 64: log logging http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@64 PS3, Line 64: kudu Kudu http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@65 PS3, Line 65: it That http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@67 PS3, Line 67: equals 5s equal to 5s http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@68 PS3, Line 68: when executes 'cluster.master()->Restart()' upon 'cluster.master()->Restart()' call http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/integration-tests/log-rolling-itest.cc@69 PS3, Line 69: which belongs that belong http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/19915/3/src/kudu/server/server_base.cc@307 PS3, Line 307: log_cleanup_interval_sec Aside from speeding up one particular test, what are the expected use cases for this configurable flag? Why would anybody modify this setting for a Kudu server running real world workloads? If there isn't any, maybe add the 'hidden' tag for this flag, so it's not even to appear in the documentation? If there are some beneficial use cases apart from speeding up a single test, then does it make sense to add the 'experimental' tag for the flag as well? -- To view, visit http://gerrit.cloudera.org:8080/19915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2be9f88c5d47bc889a4711f5aa2143b8288ba60 Gerrit-Change-Number: 19915 Gerrit-PatchSet: 3 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Fri, 28 Jul 2023 18:28:24 +0000 Gerrit-HasComments: Yes
