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 2: (14 comments) I think I addressed everything except Todd's comment about using TSAN annotations to help catch deadlocks. I need to give that some more thought. http://gerrit.cloudera.org:8080/#/c/2695/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 36: // in iteration order (i.e. the same order as TableInfo::GetAllTablets()). > Rather than saying 'iteration order', can you say that it's alphabetic orde Ah yes, I forgot that it's using std::map and not std::unordered_map. But the map keys are tablet partition start keys, not tablet IDs. To be precise, we don't _need_ alphabetical order; any order will do provided it's consistent. That is, the order should be the same with repeated calls, and, if a new element is added, it should not affect the order of the existing elements w.r.t. one another. I'll add comments. I'm less certain about the effects when a new tablet is added to an existing table, which, to be fair, isn't something we do today. What happens when one thread is iterating on N tablets and another is iterating on a slightly different order comprised of X tablets, the new tablet, and Y more tablets? Is this provably safe because the new tablet is only visible to the second thread, thus its addition can't induce a deadlock with the first thread? 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 comme I think it's the right term, though. We're talking about a tri-state lock, with READ, WRITE, and COMMIT being the states. The existing comments use that terminology. I understand that it may be confusing w.r.t. the _rest_ of Kudu, but these are the right terms for catalog_manager.cc. Anything else may be globally less confusing but locally more. I'll change it to COMMIT, maybe that will help. Line 234: // Lookup the table. > nit (while you are fixing other such nits): it's "Look up" as a verb and "l Sure. 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 regardin I named it this way to be consistent with SendAlterTableRequest which has similar semantics: there's no request for altering a table, the function sends an alter request for each tablet. 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 particu Okay. http://gerrit.cloudera.org:8080/#/c/2695/2/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 556: MasterMetadataVerifier(const unordered_set<string>& live_tables, > not sure from here or the class doc above what live and dead table sets are Yeah, I documented that in the test itself but not here. Will duplicate. Line 563: const SysTablesEntryPB& metadata) OVERRIDE { > indentation Done Line 584: Status VerifyTables() { > docs (explain what this is verifying) also a gist of this in the class head Hmm, I was hoping it'd be self-evident from the code; I'll make it explicit. But I don't want that level of detail in the class header. The consistency checks may evolve with time without anyone caring; all the caller needs to know is that Verify() will return the first inconsistency (if there is one). Line 607: return Status::Corruption("Live table set mismatch"); > would be nice to actually show the set difference here (eg dump a JoinStrin I'll show the symmetric difference so that I can get away with just one result set. Line 616: Status > docs, explain the verification algo Done Line 626: multiset<string> referenced_tables; > instead of a multiset, can you use map<string, int> here and then down belo Alright. Line 707: ColumnSchema("v1", UINT64), > are the actual columns relevant here? can you just use one of the existing Nah, the columns are irrelevant. This test suite doesn't extend any of the fancy method-rich test classes, so I'll just trim it down to one column. Line 754: ASSERT_TRUE(s.IsAlreadyPresent() || s.IsRuntimeError()) << s.ToString(); > what RuntimeError do you expect? worht a comment This changed a bit with the injected status. Hopefully it's self-explanatory now. 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); > Oh, yea, now I see why you were checking RuntimeError in the test. I agree I can defer the evaluation of the passed-in Status, but only to a point. Let me know what you think. -- 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: 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
