David Ribeiro Alves has posted comments on this change. Change subject: KUDU-495 (part 2): ensure all catalog writes for an operation are batched ......................................................................
Patch Set 2: (7 comments) still need to take a deeper look at catalog_manager.cc but though I might as well post what I have http://gerrit.cloudera.org:8080/#/c/2695/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 38: // happen in reverse (i.e. the tablet lock must be committed before the table hum can we not call this action on the lock "commit", at least in the comment?. we already overload that term more than we should. http://gerrit.cloudera.org:8080/#/c/2695/2/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: Line 579: // Send the "delete tablet request" to all replicas of all tablets of the does this just delete the tablets? i.e. does this do anything more regarding table deletion? if not maybe call this SendDeleteTabletsOfTableRequest otherwise it seems like it does more than it truly does Line 588: // Send the "delete tablet request" to a particular replica (i.e. TS and this last phrase (within the parens) is weird. maybe just "... to a particular tablet replica..." and elide whats within the parens. http://gerrit.cloudera.org:8080/#/c/2695/2/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 563: const SysTablesEntryPB& metadata) OVERRIDE { indentation Line 584: Status VerifyTables() { docs (explain what this is verifying) also a gist of this in the class header would be good (might even be enough) Line 616: Status docs, explain the verification algo http://gerrit.cloudera.org:8080/#/c/2695/2/src/kudu/util/fault_injection.h File src/kudu/util/fault_injection.h: Line 58: Status DoMaybeReturnFailure(const char* failure_str, double fraction); I think this is useful but I fear that Status::Failure is not generic enough. Can you find a way to have this return any failure status we want? This might also make sense in its own patch. -- To view, visit http://gerrit.cloudera.org:8080/2695 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5cbccf5ce22c005d7aa25bbdefe7502873a8ed7d Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
