Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797 p2: the master aggregates tablet statistics ...................................................................... Patch Set 25: (15 comments) http://gerrit.cloudera.org:8080/#/c/13426/25//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13426/25//COMMIT_MSG@13 PS25, Line 13: all the replicas are : aggregated This is no longer true, right? http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@603 PS25, Line 603: const auto RunUDF = [&] (const std::function<void()>& udf_function) { nit: I think it would be easier to follow this code if these lambdas returned the thing you actually care about. Right now, it isn't obvious without reading through each function what value is important. For example: const auto GetLeaderMaster = [&] () -> Master* { ... } const auto RunUDF = [&] (const std::function<void()>& udf_function) -> scoped_refptr<TableInfo> { ... } const auto CheckStats = [&] (TableInfo* table_info) { /* pass in the table info */ } const auto GetMasterServiceProxy = [&] () -> shared_ptr<MasterServiceProxy> { ... } That way it's clear what state is being passed around between these functions, which makes it easier to follow in my opinion. Alternatively, define a separate TsTabletManagerITest subclass that defines these as functions instead of lambdas. http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@603 PS25, Line 603: RunUDF nit: could you name this something more descriptive, like GetLeaderMasterAndRun() or something? http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@618 PS25, Line 618: check_disk_size = disk_size; : check_live_row_count = live_row_count; nit: why not just use disk_size and live_row_count directly? Also, disk_size doesn't seem to be useful, since we're always just making sure that it's >0. http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@720 PS25, Line 720: strMetricAttrs nit: metric_attrs_str for variable names. http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@724 PS25, Line 724: CheckFunction nit: you can simply pass this in as: NO_FATALS(RunUDF([&] { ... })); http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@735 PS25, Line 735: FLAGS_raft_heartbeat_interval_ms * FLAGS_leader_failure_max_missed_heartbeat_periods Since we're sleeping for these durations, how about setting these to be low values so this test doesn't take so long to run? http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@769 PS25, Line 769: strMetrics nit: metrics_str http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/integration-tests/ts_tablet_manager-itest.cc@770 PS25, Line 770: // Return the metric entity and set a retired time. : NO_FATALS(GetMetricsString(&strMetrics)); : ASSERT_STR_CONTAINS(strMetrics, kNewTableName); : // Return the metric entity and retire it. : NO_FATALS(GetMetricsString(&strMetrics)); : ASSERT_STR_CONTAINS(strMetrics, kNewTableName); : // The metric entity has been retired. : NO_FATALS(GetMetricsString(&strMetrics)); : ASSERT_STR_NOT_CONTAINS(strMetrics, kNewTableName); Hrm, I'm a bit confused about this set of assertions. Maybe I'm missing something, but the comments don't seem to match what we are doing. What's the retire time? Why does running GetMetricsString() a second time retire the entity? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@4171 PS20, Line 4171: ts_desc->ToString(), tablet->ToString(), table_schema_version, > I'm not talking about leadership changes _between masters_; I'm referring t Thinking about this more, I don't think fluctuation of a metric value is particularly harmful. It may be confusing, but maybe passable if the goal is simply monitoring. Maybe it could be more robust if we always updated stats following a leadership change on the tablet servers, since the new leader would be dirty anyway, though I'm not sure I would want that to be in this patch, particularly if the accuracy of these metrics is not critical. If we begin using the metric to do more mission-critical things like changing Impala query plans, then I would advocate for 1) not using metrics at all for that, and 2) implementing an RPC endpoint that would ensure that we have the most up-to-date value (also not a part of this patch). http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/master/master_path_handlers.cc@433 PS25, Line 433: (*output)["table_disk_size"] = : HumanReadableNumBytes::ToString(table_metrics->on_disk_size->value()); If I understand correctly, the negative case is in case the master hasn't received heartbeats for the tablet. If so, can this be negative too? Is there be a N/A case here? If not, can you add a comment explaining why we need to check one and not the other? Ah actually I think the answer is that if the table doesn't support live row counts, the value will be negative. Can you add a comment explaining that here? http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/master/table_metrics.cc File src/kudu/master/table_metrics.cc: http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/master/table_metrics.cc@26 PS25, Line 26: "Pre-replication aggregated disk space used by all tablets in this table, " : "including metadata." Hrm, why is pre-replicated disk space useful to track? The disk layout of each replica may be different, depending on compactions, so even if a user were to do something like multiply this value by the replication factor, it may not be accurate. Is that good enough for what you had in mind (like a "rough estimate" or something)? What is your goal in aggregating this? Not that I disagree, but I am curious what the utility of this is. I think it'd be useful for operators can more easily see space usage across a cluster, but if that's the goal, why not show a post-replication aggregated disk space? Is this so that we reduce the amount of heartbeating? FWIW I do agree that being a pre-replicated metric makes sense for live row count because that is a logical value that is consistent across a majority. http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/tablet/tablet_replica.cc@831 PS25, Line 831: : // The following line of code should precede "lock_", : // otherwise a deadlock will result. Hrm, I don't think I understand this. What is the deadlock here? What is lock_'s acquisition racing with? http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/tserver/ts_tablet_manager-test.cc File src/kudu/tserver/ts_tablet_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/tserver/ts_tablet_manager-test.cc@352 PS25, Line 352: while (updated_tablets != 2) { : NO_FATALS(GenerateIncrementalTabletReport(&report)); : updated_tablets = report.updated_tablets().size(); : ASSERT_TRUE(report.is_incremental()); : ASSERT_MONOTONIC_REPORT_SEQNO(&seqno, report); : } nit: Rather than doing a loop (which may not finish if something goes wrong), how about wrapping this with: ASSERT_EVENTUALLY([&] { ... ASSERT_EQ(2, report.updated_tablets().size()); }); That way, there's a time limit on the assertions passing (if i remember correctly, default 30s), and there's some incremental backoff on retrying. Same for the other loops. http://gerrit.cloudera.org:8080/#/c/13426/25/src/kudu/tserver/ts_tablet_manager-test.cc@358 PS25, Line 358: ASSERT_TRUE(report.updated_tablets(0).has_stats()); : ASSERT_TRUE(report.updated_tablets(1).has_stats()); : ASSERT_GT(report.updated_tablets(0).stats().on_disk_size(), 0); : ASSERT_EQ(0, report.updated_tablets(0).stats().live_row_count()); : ASSERT_GT(report.updated_tablets(1).stats().on_disk_size(), 0); : ASSERT_EQ(0, report.updated_tablets(1).stats().live_row_count()); nit: maybe use a loop for these so we're not duplicating a bunch of code? -- 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: 25 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 31 Jul 2019 01:41:54 +0000 Gerrit-HasComments: Yes
