Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13073 )
Change subject: hms: skip drop table notification log for external table ...................................................................... Patch Set 1: (15 comments) Sorry to do this post-review, but I broke this into two smaller patches: https://gerrit.cloudera.org/c/13314/ hms: don't overwrite table type when updating metadata https://gerrit.cloudera.org/c/13315/ hms: ignore DROP TABLE events from external tables http://gerrit.cloudera.org:8080/#/c/13073/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13073/1//COMMIT_MSG@9 PS1, Line 9: synced. > nit: could you keep the lines in this messages under 72 symbols wide? Done http://gerrit.cloudera.org:8080/#/c/13073/1//COMMIT_MSG@10 PS1, Line 10: external tables > Do we have other cases besides Kudu+Impala integration where we have such ' I think managed/external tables are a concept in Hive as well, but for all intents and purposes, only Impala-Kudu users will use these tables (for now). http://gerrit.cloudera.org:8080/#/c/13073/1//COMMIT_MSG@16 PS1, Line 16: tables > nit: the table ? Done http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/hms/hms_catalog.cc@335 PS1, Line 335: Status HmsCatalog::PopulateTable(const string& id, > Alternatively, pass in an enum that will populate the externalness in this I left it up to the caller with an enum. http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/hms/hms_catalog.cc@355 PS1, Line 355: : // TODO(hao): > What's the TODO in regards to this code block? That we should remove this c Done http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/hms/hms_catalog.cc@357 PS1, Line 357: will _be_ > The emphasis is probably better suited for _will_ rather than _be_. But I'm Yeah, I think she was just leaving open that such races will now exist. http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/hms_itest-base.h@55 PS1, Line 55: const std::string& table_type); > This can only be managed or external, right? If so, use an enum instead? Done http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/hms_itest-base.cc File src/kudu/integration-tests/hms_itest-base.cc: http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/hms_itest-base.cc@140 PS1, Line 140: if (table_type == HmsClient::kExternalTable) { : table.parameters[HmsClient::kExternalTableKey] = "TRUE"; : } > I'm not sure about I have the full context of kExternalTableKey=TRUE, but w Done I opted to keep it as is -- may be surprising to change the HMS table silently as I proposed. http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/hms_itest-base.cc@144 PS1, Line 144: // The KuduMetastorePlugin only allows the master to alter a Kudu table, : // so we pretend to be the master. > Just curious, what would be the effect of not having this? Would the reques Done http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/hms_itest-base.cc@148 PS1, Line 148: AlterTable > I'm curious whether: >it's necessary to set kKuduTableIdKey property as well? This gets filled in PopulateTable(). >this method can be successfully called multiple times producing the same >result (i.e. whether it's idempotent)? I assume so. Why do you ask? http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/integration-tests/master_hms-itest.cc@324 PS1, Line 324: ASSERT_OK(client_->OpenTable("default.e", &table)); > Might it be any race here due to the nature of synchronization between Kudu Done http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/tools/tool_action_hms.cc@169 PS1, Line 169: boost::none, > nit: can you annotate this variable with a comment? Done http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/tools/tool_action_hms.cc@171 PS1, Line 171: hms_table_copy.tableType = hms_table.tableType; > Should doc here why we need to override the tableType and parameter values Pushed this elsewhere (PopulateTable()) so we don't need to overwrite anything. http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/tools/tool_action_hms.cc@173 PS1, Line 173: hms_table_copy.parameters[HmsClient::kExternalTableKey] = "TRUE"; > Doc the workaround here too so we can grep for it and remove it later. Pushed this elsewhere (PopulateTable()) http://gerrit.cloudera.org:8080/#/c/13073/1/src/kudu/tools/tool_action_hms.cc@175 PS1, Line 175: hms_table_copy == hms_table > I'm curious whether this comparison will work after overriding the HmsClien If the kExternalTableKey isn't set in the HMS, I think it may be reasonable to consider this unsynchronized (ie the table _must_ have the `kExternalTableKey = TRUE` to be considered a valid external table). For now I've kept it as is. -- To view, visit http://gerrit.cloudera.org:8080/13073 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ffb23d507cc0d9ba9e46983e5bbf6b7116f7515 Gerrit-Change-Number: 13073 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 11 May 2019 02:32:52 +0000 Gerrit-HasComments: Yes