Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8254 )
Change subject: catalog manager: fix DDL race ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/8254/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8254/2/src/kudu/master/catalog_manager.cc@1519 PS2, Line 1519: // a concurrent rename table, try again to find the table. : TableMetadataLock lock(table_info->get(), lock_mode); > Your comment explains that we do retry, but it doesn't address my question: What error would we fail with? I don't think the error on line 1508 would be appropriate, because maybe a table with the requested name does exist. http://gerrit.cloudera.org:8080/#/c/8254/2/src/kudu/master/catalog_manager.cc@1522 PS2, Line 1522: > Hmm, that's true if lock_mode==WRITE (since writers are mutually exclusive AlterTable calls it with lock_mode==WRITE, so I don' think there's a possible race. http://gerrit.cloudera.org:8080/#/c/8254/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8254/3/src/kudu/master/catalog_manager.cc@1503 PS3, Line 1503: if (table_identifier.has_table_name()) { > Why the reversal here? Isn't lookup by ID preferable to by name? It protected against an edge-case where a client sending a TableIdentifier containing a valid table ID and an invalid table name could make FindAndLockTable infinite loop. I've changed this back and instead fixed the issue by matching on table ID and table name when both are present, and added tests accordingly. This won't have any effect on any clients. -- To view, visit http://gerrit.cloudera.org:8080/8254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ca3b207da28cd7dc43a077736da9b4e0ec6f37 Gerrit-Change-Number: 8254 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 11 Oct 2017 22:01:47 +0000 Gerrit-HasComments: Yes
