Yingchun Lai 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: (8 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 Done 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. Done 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 I just want to reduce unit test runing time, seem I have to revert it. 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 It should be 'protected' as the others, I've updated it. 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 ren No, they are table IDs. 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 t No, this field is not used, I think we needn't keep it. 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 I agree, no hurry, we can wait it. 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. Done -- 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: Sat, 18 May 2019 05:50:17 +0000 Gerrit-HasComments: Yes
