Adar Dembo has posted comments on this change. ( )

Change subject: KUDU-2191 (9/n): HMS Catalog should short-circuit no-op alter 
and create table ops

Patch Set 2:

Commit Message:
PS2, Line 9: 8
Nit: it was actually part 7 :pedantic_parrot:
File src/kudu/hms/
PS2, Line 261: The
             :   // HMS catalog will automatically short-circuit creating the 
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.
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.
File src/kudu/hms/
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?
PS2, Line 222:         return CreateOrUpdateTable(client, id, new_name, schema, 
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.
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?
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?
File src/kudu/hms/
PS2, Line 119: const char* const HmsClient::kStorageHandlerKey = 
Why duplicate this into Kudu code instead of using the constant defined in the 
HMS client generated code?

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c3594db0db253bc88e032b8c5aa2c8667c49853
Gerrit-Change-Number: 9965
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Hao Hao <>
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