Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12852 )
Change subject: [maintenance] Support priorities for tables in MM compaction ...................................................................... Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/tablet/tablet_mm_ops.h File src/kudu/tablet/tablet_mm_ops.h: http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/tablet/tablet_mm_ops.h@62 PS5, Line 62: void UpdateStats(MaintenanceOpStats* stats) OVERRIDE; If you're already cleaning this up, you could also convert 'OVERRIDE' into 'override', since C++11 supports it directly. http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager-test.cc File src/kudu/util/maintenance_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager-test.cc@77 PS5, Line 77: void SetUp() OVERRIDE { Nit: 'override' is more C++-11 compliant. http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager-test.cc@253 PS5, Line 253: op1.set_sleep_time(MonoDelta::FromMilliseconds(10)); This doesn't seem look a good change: by going from a sleep time of 1000ms to 10ms, the chance of UnregisterOp on L263 happening during the sleep decreases. Why make this change? http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager-test.cc@345 PS5, Line 345: op.set_sleep_time(MonoDelta::FromMilliseconds(10)); Likewise, does this increase the chance of the test failing, if the two remaining runs happen before we call the ASSERT_EVENTUALLY below? http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.h@233 PS5, Line 233: virtual const std::string& table_id() const = 0; This is a different visibility than the other table_id definitions in this class hierarchy. Why? http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.h@311 PS5, Line 311: typedef std::unordered_map<std::string, int32_t> TablePriorities; I assume the map keys are table names? That gets tricky when tables are renamed. Hoping the table extra config integration will address that. http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc@a531 PS5, Line 531: : : I take it you dropped this because you didn't want to worry about locking table_priorities_? In your experience, is the best op not particularly useful? http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc@100 PS5, Line 100: DEFINE_string(maintenance_manager_table_priorities, "", : "Priorities of tables, semicolon-separated list of table-priority pairs, and each " : "table-priority pair is combined by table id, colon and priority level. Priority " : "level is ranged in [-FLAGS_max_priority_range, FLAGS_max_priority_range]"); : TAG_FLAG(maintenance_manager_table_priorities, advanced); : TAG_FLAG(maintenance_manager_table_priorities, experimental); : TAG_FLAG(maintenance_manager_table_priorities, runtime); I'm not really going to review this aspect of your patch, because I assume this will move into the 'table extra config' when that's merged. http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc@342 PS5, Line 342: Status s = thread_pool_->SubmitFunc(boost::bind(&MaintenanceManager::LaunchOp, this, op)); : CHECK(s.ok()); You can combine this. http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc@467 PS5, Line 467: if (most_logs_retained_bytes_op && Could you add a comment to explain what we're looking for here? http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc@484 PS5, Line 484: if (best_perf_improvement_op && best_perf_improvement > 0) { Here too. http://gerrit.cloudera.org:8080/#/c/12852/5/src/kudu/util/maintenance_manager.cc@602 PS5, Line 602: if (!google::GetCommandLineOption("maintenance_manager_table_priorities", You can CHECK() on this returning true; this command line option is known to always exist. -- To view, visit http://gerrit.cloudera.org:8080/12852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ea3b73505157678a8fb551656123b64e6bfb304 Gerrit-Change-Number: 12852 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 17 May 2019 18:59:01 +0000 Gerrit-HasComments: Yes
