Anupama Gupta 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 14: (10 comments) Please review the changes. 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: < k > add kLogPrefix here too Done 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: > There's no need to change the namespace of this test to access stuff in the Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@149 PS12, Line 149: A > s/4/3/ Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@152 PS12, Line 152: vector<string> flags; > Please put a period (or appropriate punctuation) at the end of this sentenc Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@154 PS12, Line 154: // that is done to be able to quickly fill up the log segments in order to corrupt them > This explanation states the obvious for anyone familiar with Kudu looking a Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@166 PS12, Line 166: TestWorkload write_workload(cluster_.get()); > 5 MB seems excessive when the log segments are only 1MB. However in reality Sorry for the typo in the comment. I changed this to 32 KB to speed up the test as the maximum allowable value in this case is around 65Kb. 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 Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@227 PS12, Line 227: } > Add a comment that this needs to be at least 60 seconds to not be flaky whe Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@230 PS12, Line 230: sensusS > nit: s/Ensures/Ensure/ Done http://gerrit.cloudera.org:8080/#/c/10591/12/src/kudu/integration-tests/ts_recovery-itest.cc@238 PS12, Line 238: // that are referenced by a tablet will not be reused. > nit: remove extra blank line Done -- 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: 14 Gerrit-Owner: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 09 Jun 2018 04:35:58 +0000 Gerrit-HasComments: Yes
