Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14630 )
Change subject: [metrics] Add metric severity level ...................................................................... Patch Set 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/clock/hybrid_clock.cc@91 PS12, Line 91: kudu::MetricLevel::kInfo); > kDebug? A previous reviewer suggested I bump to kInfo so I will leave it for now. http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/clock/logical_clock.cc File src/kudu/clock/logical_clock.cc: http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/clock/logical_clock.cc@40 PS12, Line 40: kudu::MetricLevel::kInfo); > kDebug? A previous reviewer suggested I bump to kInfo so I will leave it for now. http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/master/sentry_client_metrics.cc File src/kudu/master/sentry_client_metrics.cc: http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/master/sentry_client_metrics.cc@41 PS12, Line 41: kudu::MetricLevel::kDebug); > kWarn? I think this would result in failed tasks, so it's covered by another metric. http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/tablet/tablet_metrics.cc@50 PS12, Line 50: kudu::MetricLevel::kDebug); > How about change to kInfo or even kWarn level? Given these are errors exposed to the client, I didn't think they were critical metrics except in debugging situations. http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/tablet/tablet_metrics.cc@55 PS12, Line 55: "existed.", > align, and change to kInfo level? Because update will append items to redo Done http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/tablet/tablet_metrics.cc@63 PS12, Line 63: kudu::MetricLevel::kDebug); > Change to kInfo? Because input like rows_inserted/rows_upserted are kInfo l Given there are often many concurrent scans and this is cumulative I think these are likely to be less commonly used unless in a specific debugging scenario. http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/tablet/tablet_metrics.cc@173 PS12, Line 173: kudu::MetricLevel::kDebug, > Change to kInfo? Because more delta file lookups may introduce more scan la Done http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/14630/12/src/kudu/util/metrics.cc@335 PS12, Line 335: int lowest = MetricLevelNumeric(MetricLevel::kDebug); // Default to the lowest level. : if (!filters.entity_level.empty()) { : if (MatchName(MetricLevelName(MetricLevel::kDebug), filters.entity_level)) { : lowest = MetricLevelNumeric(MetricLevel::kDebug); : } else if (MatchName(MetricLevelName(MetricLevel::kInfo), filters.entity_level)) { : lowest = MetricLevelNumeric(MetricLevel::kInfo); : } else if (MatchName(MetricLevelName(MetricLevel::kWarn), filters.entity_level)) { : lowest = MetricLevelNumeric(MetricLevel::kWarn); : } : } > How about wrap these code to a function like 'int GetMetricLevelNumeric(con I think this is okay since it's the only place it's used. -- To view, visit http://gerrit.cloudera.org:8080/14630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa7d2323bb75700104c348a3ae859fc449e1715 Gerrit-Change-Number: 14630 Gerrit-PatchSet: 12 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Thu, 14 Nov 2019 16:48:47 +0000 Gerrit-HasComments: Yes
