Adar Dembo has posted comments on this change. Change subject: master: don't expose any CreateTable() state too early ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2714/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 765: // d. Create the in-memory representation of the new table and its tablets. > I think it's worth noting here that you are creating the representation but Done Line 778: TRACE("Inserted new table and tablet info into CatalogManager maps"); > this trace is no longer correct, right? Whoops, fixed. http://gerrit.cloudera.org:8080/#/c/2714/1/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 845: // with ServiceUnavailable status. > I'm not sold on ServiceUnavailable as the code here -- usually this tells t I chose ServiceUnavailable to adhere to today's behavior in the following sequence: 1. Client 1 calls CreateTable(name=foo). 2. CreateTable() creates a new in-memory table and publishes it in the global maps, but doesn't finish creating it (i.e. state is UNKNOWN). 3. Client 2 calls GetTableSchema(foo). 4. Response to GetTableSchema() is TABLE_NOT_FOUND with ServiceUnavailable. 5. CreateTable() finishes (table is now RUNNING). Hmm, I guess I should change TABLE_ALREADY_PRESENT to TABLE_NOT_FOUND, then. Any opinions on that? And do you still think I should use Incomplete or Uninitialized for the Status? Or, if you'll allow me the use of a mutex (just for CreateTable(), never taken with the catalog manager spinlock) and a condvar, I can hide this intermediate state completely. :) Line 855: TEST_F(MasterTest, TestConcurrentCreateOfSameTable) { > point to the JIRA with a comment here There is no JIRA for the "not all master operations adhere to a memory-safe order" problem, partly because it's not a serious user issue, so my motivation for fixing it (in piecemeal) has been to remove complex abort code and simplify locking where possible. Line 862: // Kick off a thread that calls GetTableSchema() in a loop. > this doesn't seem right Whoops, copy/pasta. -- To view, visit http://gerrit.cloudera.org:8080/2714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib9e11037e7f8b4c34db5e0f2b5be00f806532365 Gerrit-PatchSet: 1 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: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
