Dan Burkert 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 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: Done 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 H It will be tested once the notification listener lands. If it weren't a short-circuit then the notification listener would get in a loop of altering the table, which would create a new alter table event, which would cause it to alter the table, ... ad infinitum. I'll add a TODO here, because I think we should eventually keep such metrics, and at that point it's worth checking. Not sure I want to roll it in to this patch, though. 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 tha Ack 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 We are now handling 'ALTER TABLE ...' and 'ALTER TABLE RENAME ...' operations a bit differently. Line 208 checks for the first case and handles it. If we fall through then we know we're in a rename operation, so we can be a bit more specific with the error message. Technically a rename operation could also alter the columns, and that would work fine here, but I don't think it's actually expressible in Hive syntax. In general I think it's good to include in the error messages if it's is a rename, because it alerts that there are two distinct table names involved instead of one. 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, No, this is actually the critical change of the patch; previously we weren't handling the case where a rename had already been applied to the HMS. Before we simply created the table, whereas now we're checking if the table already exists and updating it appropriately. This ends up being critical because the notification log can double-apply events in some cases, so we have to be able to recognize and short circuit in these cases. 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 PopulateTa Correct. Since this is now checked to be a rename operation we know it can't short-circuit. 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 Done 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 motivation to re-define it is that the name of the constant is really wide, so it makes the line wrapping in HmsCatalog really ugly. I don't think I can just reference it here, since it's defined as a std::string and we don't allow constant std::strings as far as I know. Maybe you know of a way to do that, though? I can also just remove this and go back to referencing the hive version directly if you think that's better. -- 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: 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:10:40 +0000 Gerrit-HasComments: Yes
