Adar Dembo has posted comments on this change. ( )

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

Patch Set 4:


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 
File src/kudu/integration-tests/CMakeLists.txt:
PS2, Line 79: ADD_KUDU_TEST(fuzz-itest RUN_SERIAL true)
> Nit: sort order.
I think you missed this one.
File src/kudu/integration-tests/
PS2, Line 75: op());
> Maybe use ExternalMiniClusterITestBase?
PS2, Line 79:   Status StopHms() {
> Can this be omitted?
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.
File src/kudu/master/
PS2, Line 1066:     return s;
> The order of operations in CatalogManager::Shutdown is tricky and has bitte
I think you missed this one.
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.
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.
File src/kudu/master/
PS4, Line 1561:       // TODO(dan): figure out how to test this.
You can inject faults into SysCatalogTable::SyncWrite. Maybe that will help?
PS4, Line 2253:       // TODO(dan): figure out how to test this.
See an earlier comment about injecting failures into syscatalog writes.
File src/kudu/master/hms_catalog.h:
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
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b8f360408b05d6ac4f7359a8ac0dd889b196baf
Gerrit-Change-Number: 8312
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Hao Hao <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Tue, 06 Mar 2018 19:48:33 +0000
Gerrit-HasComments: Yes

Reply via email to