[kudu-CR] KUDU-2191 (6/n): Hive Metastore catalog manager integration

["Dan Burkert (Code Review)" <ge...@cloudera.org>]

Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8312 )

Change subject: KUDU-2191 (6/n): Hive Metastore catalog manager integration
......................................................................


Patch Set 10:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@162
PS8, Line 162:   }
             : 
             :   // Checks that a table does not exist in the Kudu and HMS catalogs.
             :   void CheckCatalogsNegative(const string& database_n
> No good ideas here short of changing the MiniHMS lifecycle to outlive its m
I have spent time on trying to speed up HMS startup, to a certain extent.  That resulted in trimming a bunch of jars out of the Hadoop and Hive tarballs we vendor. [1] and [2] were the resulting artifacts.

[1] https://github.com/apache/kudu/blob/master/thirdparty/package-hadoop.sh
[2] https://github.com/apache/kudu/blob/master/thirdparty/package-hive.sh


http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@243
PS8, Line 243:   ASSERT_OK(hms_client_->CreateTable(external_table));
             : 
             :   // Attempt to rename the Kudu table to the external table name.
             :   string external_table_name = Substitute("$0.$1", hms_database_name, hms_external_table_name);
             :   unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(table_name));
             :   Status s = table_alterer->RenameTo(external_table_name)->Alter();
> Would be a good negative test for creation too, right?
Done


http://gerrit.cloudera.org:8080/#/c/8312/8/src/kudu/integration-tests/master_hms-itest.cc@268
PS8, Line 268:   ASSERT_TRUE(s.IsIllegalState()) << s.ToString();
             :   ASSERT_STR_CONTAINS(s.ToString(), "
> But isn't the CheckCatalogs() call on L277 ensuring that the Kudu table (re
1. Done
2. I agree.  I'm considering removing all of the const variables and using string literals everywhere instead.  WDYT?


http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@1755
PS9, Line 1755:         Schema schema;
> Hmm, I do think it's somewhat unintuitive; I could see someone else reading
Done


http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/catalog_manager.cc@2272
PS9, Line 2272:         Schema schema;
> Same thing.
Done


http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc
File src/kudu/master/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@201
PS9, Line 201: HMS table entry: "
             :                 << s.ToString();
             :         return create_table();
> What's stopping another HMS client (perhaps another Kudu cluster?) from renaming the entry from the original name to some other name in between L172 and L207? If that happened, we'd tell our client that the rename went through just fine when in fact it didn't happen at all.

This codepath won't be active for a rename, since orig_table and table will have different names they wont be equal.  What this codepath is protecting against is sending unnecessary add/drop column or other such alterations.

To your general point, there's absolutely nothing we can or should do to try and protect against concurrent table alterations.  Even if we sent the (no-op) alter table operation to the HMS, some other actor can come in and alter it just afterwords, before the master can ack to the Kudu client that the alter is done.  But that's fine; the operations are still strictly serialized in the HMS.


http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@258
PS9, Line 258:   // would be helpful. Perhaps a TRACE message and/or a TRACE_COUNTER_INCREMENT
> It's still an issue if Reconnect() succeeds (i.e. hms_client_->Start() succ
Ack


http://gerrit.cloudera.org:8080/#/c/8312/9/src/kudu/master/hms_catalog.cc@300
PS9, Line 300:         // Attempt to reconnect.
> No, just that in my experience, failures tend to cascade, so the first fail
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/8312
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b8f360408b05d6ac4f7359a8ac0dd889b196baf
Gerrit-Change-Number: 8312
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 03 Apr 2018 17:15:00 +0000
Gerrit-HasComments: Yes