You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "helifu (Code Review)" <ge...@cloudera.org> on 2019/08/06 12:47:55 UTC
[kudu-CR] KUDU-2797 p2: the master aggregates tablet statistics
helifu 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?
Done
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 return
Done
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 GetLeaderMasterAn
Done
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?
Done
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.
Done
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:
Done
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
I just want to check the stats after leader-follower switch. So, it's necessary to wait. ^_^
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
Done
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 som
Yeah, the process is a little confusing.
It relies on the logic of the metrics(metrics.cc Line377). I have updated the comments and hope it helps.
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,
> Thinking about this more, I don't think fluctuation of a metric value is pa
I don't think fluctuation of a metric value is particularly harmful either. And at the same time, I suggest that these aggregated metrics could be used to optimize the query plans. I know Andrew is worrying about the metrics' freshness and Adar is worrying about metrics' fluctuation, but these metrics only __affect the query plan, not the accuracy of the query results___.
In addition, in a production cluster, it's impossible to shutdown more than one tserver at the same time. That means the fluctuations tserver causes are nonexistent.But if it does, not only the metrics are inaccurate but the data are lost potentially. And for the restarted master which is the leader, the time window for all tservers to report metrics is very short.
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 r
Done
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 e
The pre-replicated disk space is used for:
1) how much disk space a record consumes;
2) how much disk space a table consumes;
And we'll use it for horizontal comparisons with other storage engines. In addition, your opinion on reducing the amount of heartbeating is another factor.
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 lo
Line1402 in raft_consensus.cc and Line606 in tablet_replica.cc
--------------------------------------------------
#4 kudu::tablet::TabletReplica::StartFollowerTransaction(scoped_refptr<kudu::consensus::ConsensusRound> const&) /mnt/ddb/2/helif/apache/kudu/src/kudu/tablet/tablet_replica.cc:606 (libtablet.so+0x23fffa)
#5 kudu::consensus::RaftConsensus::StartFollowerTransactionUnlocked(scoped_refptr<kudu::consensus::RefCountedReplicate> const&) /mnt/ddb/2/helif/apache/kudu/src/kudu/consensus/raft_consensus.cc:1044:3 (libconsensus.so+0x1892c2)
#6 kudu::consensus::RaftConsensus::UpdateReplica(kudu::consensus::ConsensusRequestPB const*, kudu::consensus::ConsensusResponsePB*) /mnt/ddb/2/helif/apache/kudu/src/kudu/consensus/raft_consensus.cc:1488:24 (libconsensus.so+0x1a34a4)
#7 kudu::consensus::RaftConsensus::Update(kudu::consensus::ConsensusRequestPB const*, kudu::consensus::ConsensusResponsePB*) /mnt/ddb/2/helif/apache/kudu/src/kudu/consensus/raft_consensus.cc:1012:14 (libconsensus.so+0x1a0880)
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
Done
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?
Done
--
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 <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 12:47:55 +0000
Gerrit-HasComments: Yes