Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15995 )
Change subject: [maintenance] use workload statistics to scale perf score of flushes/compactions ...................................................................... Patch Set 12: (4 comments) Sorry for the review delay! A couple more test suggestions, but the code is looking pretty good. http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/tserver/tablet_server-test.cc@4291 PS12, Line 4291: if (!AllowSlowTests()) { : LOG(INFO) << "Not running slow test. To run, use KUDU_ALLOW_SLOW_TESTS=1"; : return; : } nit: for future reference, this also exists as a macro SKIP_IF_SLOW_NOT_ALLOWED(); http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/tserver/tablet_server-test.cc@4335 PS12, Line 4335: SleepFor(MonoDelta::FromSeconds(60)); Could we wait for much shorter, maybe 5 seconds, to prove the same thing? If possible it's a better practice to keep tests as short as they can be while still providing valuable test coverage. That allows us to run the full test suite more frequently without using too many resources. Looking around, I suspect this was chosen because 60s is how long it takes to get time-based flushing to start. Rather than relying on this, how about doing something like: 1. Start without enabling the maintenance manager. Insert rows to the cold tablet across the keyspace and manually call the MRS flush to create some overlapping rowsets. This gives us an op that we can run with a non-0 perf score. 2. Enable the maintenance manager, with a workload score upper bound of 10, with a hot write rate of 1 row/sec, a flush threshold of 1s, and with compactions _disabled_ (--enable_rowset_compaction=false) so we can start inserting and ensure our MRS flush op starts out with the highest adjusted perf score. 3. Shut down and rebuild the tablet with the above configurations. 4. Start writing to the default tablet slowly, e.g. 1 row at a time. 5. Wait a bit to allow workload stats collection to begin. 6. Enable compactions. 7. Wait for a short amount of time, e.g. 3 seconds. 8. Verify that only the default tablet was flushed and no compactions happened. 9. Disable workload score adjustment. 10. Verify that within a couple seconds, the cold tablet's compaction will be scheduled. That way, we can still test that the workload score out-prioritizes other ops, while not taking the full 60s. http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/util/maintenance_manager-test.cc File src/kudu/util/maintenance_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/util/maintenance_manager-test.cc@557 PS12, Line 557: TestMaintenanceOp op7("op7", MaintenanceOp::HIGH_IO_USAGE, 0); : op7.set_perf_improvement(9); : op7.set_workload_score(10); : op7.set_remaining_runs(1); : op7.set_sleep_time(MonoDelta::FromMilliseconds(1)); Do we have a test anywhere that we don't run ops that have a perf score of 0, even if the workload score is high? http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: http://gerrit.cloudera.org:8080/#/c/15995/12/src/kudu/util/maintenance_manager.cc@425 PS12, Line 425: PerfImprovement nit: it's getting a bit difficult to track which perf score is which. Would you mind changing this to AdjustedPerfScore() instead? That way it's clear that when we refer to "perf improvement" we're only talking about the base perf score. -- To view, visit http://gerrit.cloudera.org:8080/15995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3afcc359002d1392164ba2fda885f8930ef8696 Gerrit-Change-Number: 15995 Gerrit-PatchSet: 12 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 31 Jul 2020 04:22:05 +0000 Gerrit-HasComments: Yes
