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 ops ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/9965/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9965/2//COMMIT_MSG@9 PS2, Line 9: 8 Nit: it was actually part 7 :pedantic_parrot: http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog-test.cc File src/kudu/hms/hms_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog-test.cc@261 PS2, Line 261: The : // HMS catalog will automatically short-circuit creating the table. Could we somehow assert that it was a short-circuit? Maybe we'd need some HMS client metrics (i.e. number of RPCs sent to HMS or something) for this, but that's probably a good idea anyway. http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog-test.cc@357 PS2, Line 357: : // Try the previous alter operation again. This should succeed, since the : // destination table ID matches, so the HMS catalog knows its the same table. : ASSERT_OK(hms_catalog_->AlterTable(kTableId, "default.ext", "default.b", schema)); Likewise, if this is supposed to short-circuit, would be nice to assert that. 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 What's the significance of the change from "altered" to "renamed" here and below? Can't AlterTable also be used to change a schema (via the 'schema' field) without renaming the table? 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_); I understand wanting to apply CreateOrUpdateTable consistently throughout, but here (and on L232 and L241) calling it means doing some unnecessary work, right? Not saying you should change this; just trying to understand. http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog.cc@244 PS2, Line 244: // Overwrite fields in the table that have changed, including the new name. In the original code, we made a copy of the table before calling PopulateTable. Was that only necessary for the short circuit check? http://gerrit.cloudera.org:8080/#/c/9965/2/src/kudu/hms/hms_catalog.cc@481 PS2, Line 481: // Otherwise we fail, since we should not create or update a non-Kudu table. Nit: it's not necessarily a non-Kudu table, right? If the storage handlers don't match then sure, but if the IDs don't match, then isn't it just not the table the client intended? 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"; Why duplicate this into Kudu code instead of using the constant defined in the HMS client generated code? -- 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: 2 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 10 Apr 2018 17:39:05 +0000 Gerrit-HasComments: Yes
