Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16220 )
Change subject: IMPALA-9989 Improve admission control pool stats logging ...................................................................... Patch Set 38: Code-Review+1 (8 comments) I didn't look through the actual functionality that closely, but generally the approach lgtm. mostly comments on code. http://gerrit.cloudera.org:8080/#/c/16220/38//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16220/38//COMMIT_MSG@7 PS38, Line 7: IMPALA-9989 Improve admission control pool stats logging this commit message is really long, consider moving some of this info in the JIRA, and shortening the commit message http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/runtime/mem-tracker.cc@459 PS38, Line 459: heavMemoryQuery nit: typo and it should be heavy_memory_query not heavMemoryQuery http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.h@553 PS38, Line 553: const PoolMetrics* metrics() const { return &metrics_; } why is this needed for and can it return a const-reference instead? http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.h@642 PS38, Line 642: public: do the two methods below need to be public? http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.h@696 PS38, Line 696: not_admitted_details("Not Applicable"), does this need to be set? http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.h@1054 PS38, Line 1054: /// A helper type to glue information together to compute the topN queries : /// out of <n> topM queries. : typedef std::tuple<int64_t, string, TUniqueId, const TPoolStats*> Item; : const int64_t& getMemConsumed(const Item& item) const { return std::get<0>(item); } : /// Get either the pool or host name. : const string& getName(const Item& item) const { return std::get<1>(item); } : /// Get the query Id. : const TUniqueId& getTUniqueId(const Item& item) const { return std::get<2>(item); } : const TPoolStats* getTPoolStats(const Item& item) const { return std::get<3>(item); } this seems like it should just be a struct, and each field in the struct should have a brief description. its also unclear what "Item" is suppose to represent. the name itself is too generic. I see it used in the vector 'listOfTopNs' below, is it suppose to be a query that consumes a lot of memory? so similar to a "heavy query"? http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16220/38/be/src/scheduling/admission-controller.cc@327 PS38, Line 327: output_indented_string nit: OutputIndentedString http://gerrit.cloudera.org:8080/#/c/16220/38/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/16220/38/common/thrift/StatestoreService.thrift@72 PS38, Line 72: num_running what is the difference between this field and num_admitted_running? -- To view, visit http://gerrit.cloudera.org:8080/16220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id995a9d044082c3b8f044e1ec25bb4c64347f781 Gerrit-Change-Number: 16220 Gerrit-PatchSet: 38 Gerrit-Owner: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 26 Aug 2020 00:30:19 +0000 Gerrit-HasComments: Yes
