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 <danburk...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 10 Apr 2018 17:39:05 +0000
Gerrit-HasComments: Yes

Reply via email to