Adar Dembo 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 Done Line 952: vector<unique_ptr<TabletMetadataLock>> tablet_locks; > do you think it would be useful to have a static method like 'LockMultipleT OK. I repurposed ScopedTabletInfoCommitter for this purpose, and settled on tablet ID order, since it's easy to articulate. Line 1005: background_tasks_->Wake(); > hrm, do we actually do anything in the 'background tasks' related to table No, this was historical. I'll remove it. 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 t As we discussed on Slack, it'll go away when the TabletReplica maps disappear. And I don't think it's particularly serious to begin with, in that failures here will be retried by the TS until they pass. 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 Done Line 556: MasterMetadataVerifier(const unordered_set<string>& live_tables, > should doc whether these are table ids or table names (or maybe rename the Will rename. Line 596: dead_visited_tables.insert(table.name); > add a comment why you don't use InsertOrDie here? it's because you're using Because it doesn't compile (returns an iterator not a pair). I agree it looks funny so I'll add a comment. Line 626: multiset<string> referenced_tables; > didnt like the suggestion of using map<string, int? and referenced_tables[t Er, I remember making that change. But it's not here. I must have blown it away accidentally; I'll make it again. 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 lik I like your approach more, though it means status_expr shouldn't contain anything that is expected to change with each call (not that it should). -- 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
