Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8254 )
Change subject: catalog manager: fix DDL race ...................................................................... Patch Set 4: (4 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: } : > sorry ignore the bit about line 1508. I don't think 'no table found' is ap As we discussed on Slack, it's important to retry (vs. just returning OK with table_info=null) to handle the following interleaving: - DELETE TABLE A - RENAME A TO B - CREATE TABLE A If the ordering is RENAME, CREATE, and DELETE, the DELETE should match and delete the table created by the CREATE. This retry enables that if the DELETE raced with the ALTER. http://gerrit.cloudera.org:8080/#/c/8254/2/src/kudu/master/catalog_manager.cc@1522 PS2, Line 1522: if (!*table_info) { > AlterTable calls it with lock_mode==WRITE, so I don' think there's a possib As we discussed on Slack, FindAndLockTable with lock_mode==READ only blocks the very end of a concurrent AlterTable, when the table lock is committed. By that point the change to table_names_map_ has already been committed, so the concurrent FindAndLockTable may or may not be exposed to it. Whether or not this is a problem is a separate issue, and I don't know the answer to that. http://gerrit.cloudera.org:8080/#/c/8254/4/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/8254/4/src/kudu/master/master-test.cc@1455 PS4, Line 1455: ASSERT_NO_FATAL_FAILURE(DoListAllTables(&tables)); Nit: NO_FATALS() for new code. http://gerrit.cloudera.org:8080/#/c/8254/4/src/kudu/master/master-test.cc@1461 PS4, Line 1461: // Delete the table with an invalid ID. Maybe do these first three cases in a loop, for terseness? Something like: for (const auto& e : { { kTableName, "abc123" }, { "abc123", table_id } ... } ) { <prep DeleteTableRequest with e.first/e.second> <assert RPC response was not found> } -- 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: 4 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: Thu, 12 Oct 2017 00:09:21 +0000 Gerrit-HasComments: Yes
