Yan-Daojiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24790 )
Change subject: [metrics] Support quantile selection in /metrics_prometheus ...................................................................... Patch Set 4: (5 comments) Thanks for the thorough review and guidance! All comments have been addressed in PS4. PTAL at your convenience. Thanks! http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.h@500 PS2, Line 500: supplied wi > Ah, I almost missed this most important piece of feedback: what if we store Good call — done. The selection is only ever populated from the statically-allocated tags in the kHistogramQuantiles table, so std::array<const char*, kNumHistogramQuantiles> is a better fit and avoids copies. BTW, membership is now compared with strcmp rather than pointer identity, because a selection may hold a caller's own tag literals (e.g. the test assigns opts.quantiles = {"0.99","0.999"}) whose addresses need not match the table's across translation units. The member is default-initialized to all-nullptr. http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.h@597 PS2, Line 597: // entity merging (see merge_rules above). > nit: could the code benefit from a typedef for this (e.g., HistogramQuantil Done http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.h@597 PS2, Line 597: // entity merging (see merge_rules above). > nit: could the code benefit from a typedef for this (e.g., HistogramQuantil Done http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.cc@170 PS2, Line 170: true; > nit: could this string be declared as a constexpr character/string literal Done — consolidated into a single source of truth reused in all three places. I used a file-local macro rather than a constexpr literal, because DEFINE_string expects a string literal token in its description, and a constexpr variable can't be spliced in there. http://gerrit.cloudera.org:8080/#/c/24790/2/src/kudu/util/metrics.cc@298 PS2, Line 298: ace { > nit: since ParseQuantiles() is being called for every request that has quan Done — switched to KLOG_EVERY_N_SECS(WARNING, 60) . -- To view, visit http://gerrit.cloudera.org:8080/24790 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I01b48aa30f26f2a7efcd842bf6512a023cbf90ff Gerrit-Change-Number: 24790 Gerrit-PatchSet: 4 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Comment-Date: Wed, 09 Sep 2026 08:36:59 +0000 Gerrit-HasComments: Yes
