Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20166 )
Change subject: MM: Give a chance to do other OP while server is under memory pressure ...................................................................... Patch Set 3: (17 comments) Thank you very much for the patch! I'm curious: are there any particular workloads that benefit a lot from the updated behavior of the 'next best op' selection? http://gerrit.cloudera.org:8080/#/c/20166/3//COMMIT_MSG Commit Message: PS3: Please follow this generic git guideline to properly format the commit message: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines You can find more useful resources at this page: https://kudu.apache.org/docs/contributing.html#_submitting_patches http://gerrit.cloudera.org:8080/#/c/20166/3//COMMIT_MSG@7 PS3, Line 7: Give a chance to do other OP while server is under memory pressure Do you have any measurements/results that show how much improvement this provides under a particular workload? Thanks! http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager-test.cc File src/kudu/util/maintenance_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager-test.cc@919 PS3, Line 919: FLAGS_not_flush_memory_prob = 1; This test scenario covers just one corner case. It would be great to add more scenarios where other values for --not_flush_memory_prob are used, at least to make sure the ratio of selected maintenance operations is as expected per the setting of the newly introduced flag. Thanks! http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@379 PS3, Line 379: pressure memory pressure http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@379 PS3, Line 379: do run http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@379 PS3, Line 379: server the server http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@380 PS3, Line 380: FlushOrNot Please describe the parameter and the return value. 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@104 PS3, Line 104: DEFINE_double(not_flush_memory_prob, 0.2, Consider adding a validator for this flag: IIUC, the only allowed values are in the range of [0, 1] http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@104 PS3, Line 104: 0.2 What sort of testing has been done to make sure setting the default value to 0.2 doesn't introduce any performance issues for existing workloads? If no testing has been performed, maybe set the default value for this parameter as 0 to maintain the legacy behavior of the new code by default? http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@105 PS3, Line 105: pressure memory pressure http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@106 PS3, Line 106: pressure memory pressure http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@107 PS3, Line 107: need to be done to run http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@519 PS3, Line 519: some performance ops What are 'performance ops'? http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@519 PS3, Line 519: even in memory pressure even if under memory pressure http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@522 PS3, Line 522: if (memory_pressure_func_(&capacity_pct) && most_logs_retained_bytes_ram_anchored_op && : FlushOrNot(capacity_pct) Would it would be easier to comprehend if calling memory_pressure_func_() from within FlushOrNot()? http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@715 PS3, Line 715: nit: the indent here should be 4 spaces per Kudu's C++ code style http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@716 PS3, Line 716: ditto -- 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: 3 Gerrit-Owner: Song Jiacheng <songjiach...@thinkingdata.cn> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng <songjiach...@thinkingdata.cn> Gerrit-Comment-Date: Wed, 12 Jul 2023 00:57:23 +0000 Gerrit-HasComments: Yes
