Ashwani Raina 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/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20166/14//COMMIT_MSG@18 PS14, Line 18: usage nit: usage is http://gerrit.cloudera.org:8080/#/c/20166/14//COMMIT_MSG@20 PS14, Line 20: this nit: the http://gerrit.cloudera.org:8080/#/c/20166/14//COMMIT_MSG@20 PS14, Line 20: higher memory nit: higher the memory http://gerrit.cloudera.org:8080/#/c/20166/14//COMMIT_MSG@20 PS14, Line 20: flush : MRS/DMS nit: MRS/DMS flushes http://gerrit.cloudera.org:8080/#/c/20166/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20166/17//COMMIT_MSG@7 PS17, Line 7: not always flush even if nit: Avoid unchecked scheduling of flush operations http://gerrit.cloudera.org:8080/#/c/20166/17//COMMIT_MSG@20 PS17, Line 20: this higher probability to do flush nit: higher the probability to flush http://gerrit.cloudera.org:8080/#/c/20166/17//COMMIT_MSG@20 PS17, Line 20: The higher nit: Higher the http://gerrit.cloudera.org:8080/#/c/20166/17//COMMIT_MSG@27 PS17, Line 27: nit: remove whitespace http://gerrit.cloudera.org:8080/#/c/20166/17//COMMIT_MSG@27 PS17, Line 27: not_flush_memory_prob nit: you mean "run_non_memory_ops_prob" ? 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@110 PS17, Line 110: maintainer nit: maybe you could use "system admin" or "user"? There has to be some mechanism by which user/admin is made aware that this flag can be used to regulate the scheduling of flush operations when node is under memory pressure. Maybe you could point to some (metrics) or (recommended action via a warning log), visible to user/admin that essentially denote these conditions and suggest to turn this on or increase the probability. http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@111 PS17, Line 111: the performance is decreasing. nit: "there is a significant degradation in performance." http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@730 PS17, Line 730: FlushOrNot FlushOrNot becomes confusing when return type is bool. It is not clearly evident from the name whether this method returns true or false when flush is a go. Maybe rename to "ProceedWithFlush" ? http://gerrit.cloudera.org:8080/#/c/20166/17/src/kudu/util/maintenance_manager.cc@730 PS17, Line 730: bool MaintenanceManager::FlushOrNot(double* used_memory_percentage) { nit: Add a comment above that describes the purpose of this method, out parameter and return value. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Comment-Date: Tue, 12 Sep 2023 07:30:14 +0000 Gerrit-HasComments: Yes
