Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10591 )
Change subject: KUDU-1038 Deleting a tablet should also delete its log recovery directory, if any ...................................................................... Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/consensus/log.cc@997 PS12, Line 997: < " add kLogPrefix here too http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc File src/kudu/integration-tests/recover_tablet-itest.cc: http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@133 PS7, Line 133: > Done. Although the test works even for cases when the deleted log segment i OK, feel free to riff on that, it was just a suggestion of the type of comment that will be useful to people reading the code in the future. http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc File src/kudu/integration-tests/ts_recovery-itest.cc: http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@88 PS12, Line 88: namespace tserver { There's no need to change the namespace of this test to access stuff in the tserver namespace (just add a using directive for anything you need to import) http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@149 PS12, Line 149: 4 s/4/3/ http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@152 PS12, Line 152: // Start a cluster with 3 tablet servers consisting of 1 tablet with 3 replicas Please put a period (or appropriate punctuation) at the end of this sentence, and every other comment sentence in this patch. http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@154 PS12, Line 154: // The log segment size and the number of log segments to retain is configured to be very small This explanation states the obvious for anyone familiar with Kudu looking at the code. Please explain why we are doing that. The reason is so that we will be able to quickly fill up several log segments in order to corrupt them in a way that exercises the necessary code paths for this regression test. http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@166 PS12, Line 166: write_workload.set_payload_bytes(5 * 1024); // Write ops of size 5MB to quickly fill the logs. 5 MB seems excessive when the log segments are only 1MB. However in reality these ops are configured to be 5KB here. Consider making them 512 * 1024 bytes to speed up the test. http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@171 PS12, Line 171: nit: leave only one blank line, not two http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@227 PS12, Line 227: WaitUntilTabletRunning(ts_details, tablet_id, MonoDelta::FromSeconds(60))); Add a comment that this needs to be at least 60 seconds to not be flaky when running in TSAN mode. http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@230 PS12, Line 230: Ensures nit: s/Ensures/Ensure/ http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@238 PS12, Line 238: nit: remove extra blank line -- To view, visit http://gerrit.cloudera.org:8080/10591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If3f017caf41ac4e00bf89421d6e73924b9fdcbd0 Gerrit-Change-Number: 10591 Gerrit-PatchSet: 12 Gerrit-Owner: Anupama Gupta <[email protected]> Gerrit-Reviewer: Anupama Gupta <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 08 Jun 2018 18:40:56 +0000 Gerrit-HasComments: Yes
