Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22314 )
Change subject: KUDU-3563 Output tablet-level metrics in Prometheus ...................................................................... Patch Set 7: (18 comments) http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/master/master-test.cc@3840 PS7, Line 3840: * style nit: asterisk goes with the type http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/master/master-test.cc@3840 PS7, Line 3840: const nit: could this this be static constexpr? http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/master/master-test.cc@3854 PS7, Line 3854: output nit: could this be just 'buf.ToString()'? http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/master/master-test.cc@3854 PS7, Line 3854: kudu:: nit: I'd expect this prefix isn't needed since this is already in kudu::master namespace http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/tablet/tablet_metrics.cc@129 PS7, Line 129: "to the Scanner Cells Returned metric." nit: add a space after the dot http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/tablet/tablet_metrics.cc@141 PS7, Line 141: "to the Scanner Bytes Returned metric." nit: ditto http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/tserver/tablet_server-test.cc@4362 PS7, Line 4362: output nit: could this be just 'buf.ToString()'? http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/tserver/tablet_server-test.cc@4362 PS7, Line 4362: kudu:: nit: I'd think this prefix isn't necessary since this code is already in the kudu::tserver namespace http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/metrics-test.cc@176 PS7, Line 176: ASSERT_EQ nit: in ASSERT_EQ() the expected comes first -- otherwise, it's quite hard to comprehend the error message when the assertion triggers http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/metrics.cc@440 PS7, Line 440: } else { : const string prefix = Substitute("kudu_$0_$1_", prototype_->name(), id_); : WriteMetricsPrometheus(writer, metrics, prefix); : return Status::OK(); nit: 'else' is redundant since there is 'return' in the 'if()' clause above http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.h@221 PS7, Line 221: std::string nit: could this be passed by reference? http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.h@221 PS7, Line 221: checkPrometheusOutput style nit: method names should start with capital letter unless that are property getters/setters and alike http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc@724 PS7, Line 724: std:: nit here and below: remove the redundant 'std::' namespace prefix since there are corresponding 'using ...' in the beginning of this file http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc@729 PS7, Line 729: while ((pos = prometheus_output.find('\n')) != std::string::npos) { : lines.push_back(prometheus_output.substr(0, pos)); : prometheus_output.erase(0, pos + 1); : } Maybe, use strings::Split() instead? http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc@747 PS7, Line 747: std::unordered_set<string> metric_names; nit for the pattern matching below: consider using ASSERT_STR_MATCHES() -- it allows for regex patterns, and in most cases it's more expressive and easier to understand at the same time. http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc@750 PS7, Line 750: ASSERT_EQ nit: with ASSERT_EQ(), the expected comes the first, so it's much easier to comprehend the error message if the assertion ever triggers http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc@755 PS7, Line 755: << nit here and below: add spaces to separate the operator http://gerrit.cloudera.org:8080/#/c/22314/7/src/kudu/util/test_util.cc@760 PS7, Line 760: nit: wrong indent? -- To view, visit http://gerrit.cloudera.org:8080/22314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id033a1410966167d9bb24f6a44c584e73c17a4af Gerrit-Change-Number: 22314 Gerrit-PatchSet: 7 Gerrit-Owner: Ádám Bakai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Tue, 28 Jan 2025 07:20:50 +0000 Gerrit-HasComments: Yes
