Todd Lipcon has posted comments on this change. Change subject: KUDU-495 (part 2): ensure all catalog writes for an operation are batched ......................................................................
Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/2695/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 945: Substitute dont need substitute Line 952: vector<unique_ptr<TabletMetadataLock>> tablet_locks; do you think it would be useful to have a static method like 'LockMultipleTablets(...)' which handles acquiring the locks on a bunch of tablets together in a "safe" order? ie it could sort the tablets by ID (or pointer) first before acquiring the locks? or at least assert that they are in whatever order we pick? Line 1005: background_tasks_->Wake(); hrm, do we actually do anything in the 'background tasks' related to table deletion? Line 1599: // TODO: we should undo the in-memory tablet replica locations changes made above. is this a serious issue? if so should probably file a JIRA about it under the multi-master stuff? http://gerrit.cloudera.org:8080/#/c/2695/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 544: // Verifies that on-disk master metadata is consistent. self-consistent and matches a set of expected contents Line 556: MasterMetadataVerifier(const unordered_set<string>& live_tables, should doc whether these are table ids or table names (or maybe rename the args and member vars) Line 596: dead_visited_tables.insert(table.name); add a comment why you don't use InsertOrDie here? it's because you're using names and thus expect you could have dups right? Line 626: multiset<string> referenced_tables; didnt like the suggestion of using map<string, int? and referenced_tables[tableid]++? http://gerrit.cloudera.org:8080/#/c/2695/3/src/kudu/util/fault_injection.h File src/kudu/util/fault_injection.h: Line 53: fraction_flag, status_expr)) \ you could avoid constructing the status in all cases by doing something like: static const Status _s = (status_expr); DoMaybeReturnFailure(fraction_flag, _s) and make the function take a const Status& though it probably doesn't matter, since you've already made it so it's only evaluated for the case when injection's enabled, and we probably don't care about perf there :) -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: 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
