[kudu-CR] hms: skip drop table notification log for external table

["Andrew Wong (Code Review)" <ge...@cloudera.org>]

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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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