Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet metrics ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.h@194 PS1, Line 194: uint64_t row_counts(); : uint64_t disk_size(); > Nope, they are pre-replication. And it does not add up. The variable 'metri Post-replication metrics make sense, but pre-replication are a little weird for exactly that reason: if you display just the first entry of metrics_, how do you know that the remaining entries aren't wildly different? Some of the metrics we might choose to aggregate (like disk size) are physical properties, and thus will differ across replicas of the same tablet. I'm curious to hear Mike's thoughts on this patch, since he suggested aggregation in the first place. http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/heartbeater.cc@77 PS1, Line 77: : DEFINE_int32(heartbeat_metrics_interval_ms, 5000, : "Interval at which the TS metrics to the master."); : TAG_FLAG(heartbeat_metrics_interval_ms, advanced); > Yea, I have thought about it. But, right now the 'WRITE_OP' doesn't dirty t Hmm, dirtying on each write would be expensive too. Maybe a two-period approach would be best: 1. The first period defines the maximum amount of time between when a metric's value changes and when the heartbeater "notices". 2. The second period is heartbeat_interval_ms, and defines the amount of time between heartbeats. The idea would be to periodically (1) check whether the metrics value has changed (provided it's cheap to do so), and if it has, dirty the tablet so that when the heartbeater next runs (2) an incremental report including this tablet (and its metrics) is sent. What do you think? I'll also ask Mike to comment since he suggested this style of aggregation in the first place. http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/ts_tablet_manager.cc@1276 PS1, Line 1276: CountRows > It seems that the calls to "CountRows()" is not safe(OnDiskSize is safe!) e Won't CountRows() initialize the cfile reader? And isn't that initialization "one way"? Meaning, once initialized, a cfile reader stays initialized for the lifetime of the process? Not seeing the issue here. http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/ts_tablet_manager.cc@1276 PS1, Line 1276: CHECK_OK(replica->tablet()->CountRows(&count_rows)); > * Ok. Yeah I'm curious if we can track deletes without incurring IO. I'm concerned that it basically boils down to a full tablet scan with an empty projection, which would be super expensive. -- To view, visit http://gerrit.cloudera.org:8080/13426 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74406ab7cca7c22fda455c328b8ee9989a6b2d99 Gerrit-Change-Number: 13426 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Mon, 27 May 2019 22:20:44 +0000 Gerrit-HasComments: Yes
