Adar Dembo 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 4:

(10 comments)

Just responding to your responses, will defer a rereview until the tests pass 
(it's been long enough since my first review that I've forgotten all of the new 
code).

http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/CMakeLists.txt@79
PS2, Line 79: ADD_KUDU_TEST(fuzz-itest RUN_SERIAL true)
> Nit: sort order.
I think you missed this one.


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

http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@75
PS2, Line 75: op());
> Maybe use ExternalMiniClusterITestBase?
Done


http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@79
PS2, Line 79:   Status StopHms() {
> Can this be omitted?
Done


http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/integration-tests/master_hms-itest.cc@112
PS2, Line 112:     b.AddColumn("decimal32_val")->Type(KuduColumnSchema::DECIMAL)
> ???
The table_name() function expects a cref string, so std::move(table_name) has 
no effect.


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

http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1066
PS2, Line 1066:     return s;
> The order of operations in CatalogManager::Shutdown is tricky and has bitte
I think you missed this one.


http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@1598
PS2, Line 1598:   }
> Why is it OK if this succeeds but step 3 fails? I can take my answer in the
Hmm, the new behavior treats HMS table entry dropping as fatal, so this new 
comment is no longer correct.


http://gerrit.cloudera.org:8080/#/c/8312/2/src/kudu/master/catalog_manager.cc@2092
PS2, Line 2092:       case AlterTableRequestPB::DROP_RANGE_PARTITION: {
> Shouldn't some aspect of this be conditioned on one (or several) of has_foo
I think you missed this.


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

http://gerrit.cloudera.org:8080/#/c/8312/4/src/kudu/master/catalog_manager.cc@1561
PS4, Line 1561:       // TODO(dan): figure out how to test this.
You can inject faults into SysCatalogTable::SyncWrite. Maybe that will help?


http://gerrit.cloudera.org:8080/#/c/8312/4/src/kudu/master/catalog_manager.cc@2253
PS4, Line 2253:       // TODO(dan): figure out how to test this.
See an earlier comment about injecting failures into syscatalog writes.


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

http://gerrit.cloudera.org:8080/#/c/8312/3/src/kudu/master/hms_catalog.h@29
PS3, Line 29: #include "kudu/util/net/net_util.h"
> optional seems to require the include in order to compile.
Not using optional anymore?



--
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: 4
Gerrit-Owner: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Tue, 06 Mar 2018 19:48:33 +0000
Gerrit-HasComments: Yes

Reply via email to