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 12: (23 comments) Thanks for the useful comments Mike. I have addressed all of them. http://gerrit.cloudera.org:8080/#/c/10591/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10591/7//COMMIT_MSG@18 PS7, Line 18: > after the next tablet copy. Done http://gerrit.cloudera.org:8080/#/c/10591/10/src/kudu/consensus/log.h File src/kudu/consensus/log.h: http://gerrit.cloudera.org:8080/#/c/10591/10/src/kudu/consensus/log.h@145 PS10, Line 145: * > nit: don't make these changes, keep the modifiers next to the type to be co Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.h File src/kudu/consensus/log.h: http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.h@143 PS7, Line 143: , > add: " if it exists" Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.h@145 PS7, Line 145: RemoveRecoveryDir > How about renaming this to RemoveRecoveryDirIfExists() ? Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.cc@996 PS7, Line 996: !fs_ > nit: this syntax is neat, but for consistency with the rest of the Kudu C++ Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.cc@997 PS7, Line 997: b > nit: extra space, also make this a VLOG(1) to avoid spamming the server log Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.cc@1002 PS7, Line 1002: con > All of these logs should also log the prefix "T <tablet id> P <peer uuid>: Makes sense. Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.cc@1005 PS7, Line 1005: str > All of these LOG messages used to be VLOG_WITH_PREFIX(1) and I think we sho Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/consensus/log.cc@1006 PS7, Line 1006: VLOG(1) << kLogPrefi > nit: line up your indentation with the previous << here and elsewhere Done 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@59 PS7, Line 59: > nit: Regression test for KUDU-1038. Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@60 PS7, Line 60: > or is not fsynced before a crash Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@61 PS7, Line 61: : : > slight rewrite: Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@64 PS7, Line 64: : > suggestion: Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@66 PS7, Line 66: > Can you move this test to ts_recovery-itest and make this part of TsRecover Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@79 PS7, Line 79: > This actually just creates a new tablet. Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@87 PS7, Line 87: > nit: missing trailing period per style guide per https://google.github.io/s Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@96 PS7, Line 96: : : > you can get rid of this Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@107 PS7, Line 107: > remove Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@111 PS7, Line 111: > remove this debugging log message Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@117 PS7, Line 117: > add a period at the end of your comment sentences, here and elsewhere in th Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@133 PS7, Line 133: > This comment describes the next line of code and is basically superfluous, Done. Although the test works even for cases when the deleted log segment is at index 1. http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@141 PS7, Line 141: > remove this comment Done http://gerrit.cloudera.org:8080/#/c/10591/7/src/kudu/integration-tests/recover_tablet-itest.cc@144 PS7, Line 144: > remove this sleep 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: 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 17:15:27 +0000 Gerrit-HasComments: Yes
