Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20166 )
Change subject: KUDU-3407 not always flush even if under memory pressure. ...................................................................... Patch Set 17: (13 comments) http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager-test.cc File src/kudu/util/maintenance_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager-test.cc@151 PS17, Line 151: DLOG(INFO) << "Re-registering op " << this->name(); Consider removing this once done debugging/troubleshooting the new test -- it isn't used by the test, but pollutes the output. http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager-test.cc@314 PS17, Line 314: int64_t nit: why not to use the MonoDelta type here? http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager-test.cc@315 PS17, Line 315: Perform count; nit: How many times the operation has been run. http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager-test.cc@989 PS17, Line 989: not assert anything Is it possible to assert on the range of the expected counters, etc.? Given the amount of memory is pre-determined and doesn't depend on the actual amount of memory that a node has, I'd expect that the scenario should preserve some particular traits even if its behavior is driven by some stochastic factors. Running the scenario multiple times should converge to a pretty narrow range of the result counters, no? http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.h@379 PS17, Line 379: server pressure the memory pressure http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.h@386 PS17, Line 386: bool FlushOrNot(double* used_memory_percentage); nit: you could use a free format in a non-public API when documenting methods and fields; the doxygen format isn't a requirement here http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@522 PS3, Line 522: // Look at ops that we can run quickly that free up log retention. : if (low_io_most_logs_retai > That would be much better. Alternatively, updating the `memory_pressure_func_` with the new function could be a way to go. Basically, having less parts for the condition would be better. http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@105 PS17, Line 105: we nit: Who's "we" here? It's better to be more specific on who does what here. http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@108 PS17, Line 108: ran run http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@136 PS17, Line 136: Simulated memory usage What is the unit of the usage? http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@539 PS17, Line 539: in under http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@542 PS17, Line 542: FlushOrNot Alternatively, consider calling memory_pressure_func_() functor as it is, but assign it to a different method/function, as necessary? http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@732 PS17, Line 732: if (PREDICT_FALSE(FLAGS_memory_simulate_for_test != 0.0)) { : *used_memory_percentage = FLAGS_memory_simulate_for_test; : } Instead of introducing this extra test flag and adding this extra logic, consider setting the memory_pressure_func_ via the MaintenanceManager::set_memory_pressure_func_for_tests() method for specific tests, similar to what's done in MaintenanceManagerTest::StartManager()? The extra level of indirection introduced by MaintenanceManager::memory_pressure_func_ is there just for this particular purpose, so introducing an extra flag looks like an overkill if having that provision already. -- To view, visit http://gerrit.cloudera.org:8080/20166 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc2fd3a850cf99d54ef2980211b712468440ed80 Gerrit-Change-Number: 20166 Gerrit-PatchSet: 17 Gerrit-Owner: Song Jiacheng <songjiach...@thinkingdata.cn> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng <songjiach...@thinkingdata.cn> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Wed, 06 Sep 2023 23:46:58 +0000 Gerrit-HasComments: Yes
