Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9965 )
Change subject: KUDU-2191 (9/n): HMS Catalog should short-circuit no-op alter and create table calls ...................................................................... Patch Set 3: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog.cc@218 PS2, Line 218: renamed > We are now handling 'ALTER TABLE ...' and 'ALTER TABLE RENAME ...' operati Makes sense; I missed the L208 short-circuit and thought these code paths were for all ALTERs. http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog.cc@222 PS2, Line 222: return CreateOrUpdateTable(client, id, new_name, schema, master_addresses_); > No, this is actually the critical change of the patch; previously we weren' Yes, I figured it was important to call CreateOrUpdateTable(), was just wondering whether _some_ of the work it did was duplicated. On second look though, there is a key difference between the calls to ParseTableName() and GetTable() here and in CreateOrUpdateTable(): the calls here use the old table name while the calls in CreateOrUpdateTable() use the new table name. So they are completely different and there's no duplicate work. http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_client.cc@119 PS2, Line 119: const char* const HmsClient::kStorageHandlerKey = "storage_handler"; > The motivation to re-define it is that the name of the constant is really w I don't feel strongly, I just wanted to know whether this was a cosmetic thing as I suspected it was, or whether there was a different reason. -- To view, visit http://gerrit.cloudera.org:8080/9965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c3594db0db253bc88e032b8c5aa2c8667c49853 Gerrit-Change-Number: 9965 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 10 Apr 2018 18:23:44 +0000 Gerrit-HasComments: Yes
