Song Jiacheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20166 )
Change subject: KUDU-3407: Give a chance to do other maintenance operations while server is under memory pressure. ...................................................................... Patch Set 5: (17 comments) > Patch Set 5: Verified-1 > > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/28183/ : FAILURE This unit test TestPrioritizeLogRetentionUnderMemoryPressure works well in my PC. http://gerrit.cloudera.org:8080/#/c/20166/3//COMMIT_MSG Commit Message: PS3: > Please follow this generic git guideline to properly format the commit mess Changed the tittle and still not sure it's ok. http://gerrit.cloudera.org:8080/#/c/20166/3//COMMIT_MSG@7 PS3, Line 7: -3407: Give a chance to do other maintenance operations while serv > Do you have any measurements/results that show how much improvement this pr This patch has been working in our clusters(more than 100 clusters) for over half a year. It does make some influence since the performance will not be too low even if the cluster stay in high memory pressure for very long. According to web page and log, tablet server is scheduling some other operations like CompactRowsetOp, MajorCompactOp, which have big impact on the performance of tablet server. 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 m About the test, there is another test about the whole maintenance manager, testing which operations will be run in various policies, which you mentioned in JIRA. But it's based on this patch so I post this patch first. 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: ressure > memory pressure Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@379 PS3, Line 379: ends o > the server Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@379 PS3, Line 379: r > run Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.h@380 PS3, Line 380: he flag no > Please describe the parameter and the return value. Done 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: 0, > What sort of testing has been done to make sure setting the default value t That's the default value in our clusters and it works well, but still I changed it to 0. 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, > Consider adding a validator for this flag: IIUC, the only allowed values ar Added validator for not_flush_memory_prob and also data_gc_prioritization_prob. http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@105 PS3, Line 105: " > memory pressure Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@106 PS3, Line 106: server > memory pressure Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@107 PS3, Line 107: d there are oth > to run Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@519 PS3, Line 519: > even if under memory pressure Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@519 PS3, Line 519: > What are 'performance ops'? Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@522 PS3, Line 522: // anchors the most WALs (the op should also free memory). : // > Would it would be easier to comprehend if calling memory_pressure_func_() f That would be much better. http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@715 PS3, Line 715: void Main > nit: the indent here should be 4 spaces per Kudu's C++ code style Done http://gerrit.cloudera.org:8080/#/c/20166/3/src/kudu/util/maintenance_manager.cc@716 PS3, Line 716: running > ditto Done -- 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: 5 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 12 Jul 2023 09:04:37 +0000 Gerrit-HasComments: Yes
