subject:"\[kudu\-CR\] KUDU\-2191\: Metadata Upgrade Tool"

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Hao Hao (Code Review)

Hao Hao has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Reviewed-on: http://gerrit.cloudera.org:8080/10075
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 775 insertions(+), 59 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 21
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 20: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 20
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 19 May 2018 00:37:57 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 20:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/kudu-tool-test.cc@2043
PS19, Line 2043:
> repeated just below
Done


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@69
PS19, Line 69: DEFINE_bool(enable_input, true,
> I think true would be a better default here, since it's more annoying to ha
Done


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@124
PS19, Line 124: Found orphan table $
> nit: 'Found orphan table $0.$1...'
Done


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@159
PS19, Line 159: Failed to upgrade legacy Impala table
> I think something along the lines of
Done


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@165
PS19, Line 165: } else {
> I think this lambda is obscuring things a bit.  For instance the SchemaFrom
Done


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@170
PS19, Line 170:   while (!s.ok() && FLAGS_enable_input &&
> Would be good to have a test of this loop, just to have some coverage of th
Done


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@188
PS19, Line 188:   if (!failures.empty()) {
> LOG(WARNING) the other errors so they get surfaced
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 20
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 19 May 2018 00:15:34 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#20).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 775 insertions(+), 59 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/20
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 20
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 19:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/kudu-tool-test.cc@2043
PS19, Line 2043: One with hive incompatible name.
repeated just below


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@69
PS19, Line 69: DEFINE_bool(enable_input, false,
I think true would be a better default here, since it's more annoying to have 
to set flags in interactive mode than in batch mode typically.


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@124
PS19, Line 124: Find an orphan table
nit: 'Found orphan table $0.$1...'


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@159
PS19, Line 159: Table $0 already exists before upgrade
I think something along the lines of

'Failed to upgrade legacy Impala table 'foo.bar' (Kudu table name: 'fuzz') 
because a Kudu table with name 'foo.bar' already exists'

would be more clear


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@165
PS19, Line 165:   auto create_hms_table = [&](const string& name) -> Status 
{
I think this lambda is obscuring things a bit.  For instance the 
SchemaFromKuduSchema call really only needs to happen once, and then after that 
it's a pretty simple function call.


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@170
PS19, Line 170:   while (!s.ok() && FLAGS_enable_input &&
Would be good to have a test of this loop, just to have some coverage of the 
match, since it's brittle WRT Hive changing error messages.


http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@188
PS19, Line 188: return failures.front();
LOG(WARNING) the other errors so they get surfaced



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 19
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 18 May 2018 18:51:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 19: Verified+1

(9 comments)

Unrelated flaky test failure.

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@21
PS16, Line 21: #include 
> looks like this is no longer needed?  Curious that IWYU didn't catch this.
Yeah, it looks like this file is filtered to not run 
IWYU?https://github.com/apache/kudu/blob/master/build-support/iwyu.py#L65


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@103
PS16, Line 103: SL gflags.
> I'd drop this last part of the sentence, I think it's clear from the signat
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@75
PS16, Line 75: "table should reside 
in";
> Might be simpler to return the map instead of taking it as an out param.
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@121
PS16, Line 121: bool exist;
> Check the return status.
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@123
PS16, Line 123: if (!exist) {
> This could cause problems for installations which have two Kudu clusters po
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@135
PS16, Line 135:
> What do you think about re-organizing this so that the failure to upgrade a
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@146
PS16, Line 146:   if (hms_table->parameters[HmsClient::kStorageHandlerKey] 
==
> Maybe worth doing a check that 'new_table_name' isn't contained in table_na
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@148
PS16, Line 148: string new_table_name = Substitute("$0.$1", 
hms_table->dbName, hms_table->tableName);
> Since this will probably land before the listener, just add a TODO for me t
Done


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@156
PS16, Line 156: 
client::SchemaFromKuduSchema(kudu_table->schema()));
> As we discussed offline, I think we should just assume the table has alread
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 19
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 18 May 2018 18:29:43 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Hao Hao (Code Review)

Hao Hao has removed a vote on this change.

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 19
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-18 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#19).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 754 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/19
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 19
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-17 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#18).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 759 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/18
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 18
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-17 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#17).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 762 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/17
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 17
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-17 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 16:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@148
PS16, Line 148: // If UpgradeLegacyImpalaTable fails, there will be an 
orphan table in the HMS.
> Perhaps we could use the notification listener to watch for these upgrade e
Since this will probably land before the listener, just add a TODO for me to 
look at this once that lands.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 17 May 2018 23:35:47 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-17 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 16:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@21
PS16, Line 21: #include 
looks like this is no longer needed?  Curious that IWYU didn't catch this.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@103
PS16, Line 103:  and returns in a vector
I'd drop this last part of the sentence, I think it's clear from the signature.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@75
PS16, Line 75: void RetrieveTablesMap(vector hms_tables,
Might be simpler to return the map instead of taking it as an out param.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@121
PS16, Line 121: kudu_client->TableExists(hms_table.first, );
Check the return status.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@123
PS16, Line 123:   
RETURN_NOT_OK(hms_catalog->DropLegacyImpalaTable(hms_table.second.dbName,
This could cause problems for installations which have two Kudu clusters 
pointed at the same HMS.  I know we aren't explicitly supporting that for the 
first cut, but it may be better to warn here instead of dropping, just so 
things don't break.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@135
PS16, Line 135:   for (const auto& table_name : table_names) {
What do you think about re-organizing this so that the failure to upgrade a 
table doesn't prevent all subsequent tables from being upgraded?  Probably 
requires building up a list of Status's which have failed.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@146
PS16, Line 146: string new_table_name = Substitute("$0.$1", 
hms_table->dbName, hms_table->tableName);
Maybe worth doing a check that 'new_table_name' isn't contained in table_names 
already?  Since that would mean a rename conflict.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@148
PS16, Line 148: // If UpgradeLegacyImpalaTable fails, there will be an 
orphan table in the HMS.
Perhaps we could use the notification listener to watch for these upgrade 
events, and rename the table automatically?  That would close most of the race 
condition here.


http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@156
PS16, Line 156:   string new_table_name = Substitute("$0.$1", 
default_database,
As we discussed offline, I think we should just assume the table has already 
been renamed by this tool.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 17 May 2018 23:31:53 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-17 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#16).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 753 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/16
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-16 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 14:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100
PS12, Line 100:   const std::string& tb_name,
> OK, but how can this method know the Kudu table name of an HMS table entry?
Done


http://gerrit.cloudera.org:8080/#/c/10075/13/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/13/src/kudu/tools/tool_action_hms.cc@88
PS13, Line 88: }
> This is the case of a non-Impala legacy table, right?  I thought this tool
I updated the tool to rename the table to be Hive-compatible while upgrade.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 17 May 2018 02:14:33 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-16 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#14).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. To run this tool, it
requires to turn off the HMS integration feature. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tools/tool_test_util.cc
M src/kudu/tools/tool_test_util.h
26 files changed, 755 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/14
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-14 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100
PS12, Line 100:   // This method will fail if the HMS is unreachable.
> In the following patch, the same method is used to retrieve non-legacy tabl
OK, but how can this method know the Kudu table name of an HMS table entry?  If 
you don't have knowledge of the storage handler, there's no single way to 
determine the Kudu table name, or even if it's a Kudu table.  The current 
implementation just hard-codes the knowledge in a way that's only relevant to 
Impala legacy tables.  In general I think this is just too specialized of a 
method for this API, which tends to be somewhat more generic.  It'd be better 
if it returned a vector of the tables which match the storage handler, and let 
the caller do the special post-processing which is pretty specific to the 
calling context.


http://gerrit.cloudera.org:8080/#/c/10075/13/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/13/src/kudu/tools/tool_action_hms.cc@88
PS13, Line 88:   new_table_name = Substitute("$0.$1", default_database, 
table_name);
This is the case of a non-Impala legacy table, right?  I thought this tool 
assumed the table had already been renamed to be Hive-compatible via the new 
table rename tool?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 14 May 2018 22:52:24 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-10 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 13:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc
File src/kudu/hms/hms_catalog-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@237
PS12, Line 237: table.__set_parameters({
> I think it would be better to remove this behavior from the KuduMetastorePl
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@456
PS12, Line 456: hms_tables_map, kExternalTableName));
> instead of calling GetTable and passing in the parameter, perhaps it's bett
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@91
PS12, Line 91:   const std::string& db_name,
> This is the name of the table as stored in the HMS, right?  If so, it might
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@96
PS12, Line 96: or a
> nit: 'is the legacy...'
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100
PS12, Line 100:   // This method will fail if the HMS is unreachable.
> I'm not quite following this bit - why is storage_handler taken as an argum
In the following patch, the same method is used to retrieve non-legacy tables. 
So it takes storage_handler as an argument. The reason why the need to have a 
map is to avoid O(n) table lookup given a Kudu table's name.


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc@193
PS12, Line 193:
> nit: 'non-legacy'
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto@43
PS12, Line 43:   // Whether or not to create a Hive Metastore, and/or enable 
Kudu Hive
 :   // Metastore integration.
 :   optional HmsMode hms_mode = 7;
 :
 :   // The directory where the cluster's data and l
> Same feedback.
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@67
PS12, Line 67: non-Impala
> 'non-Impala' here and below (non should usually be hyphen separated from th
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@136
PS12, Line 136: 
> This should probably go before creating the HmsCatalog.
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@145
PS12, Line 145:   .master_server_addrs(master_addresses)
> Maybe add a comment here?  I'm not sure why this would need to have non-def
Oops, thanks for pointing it out. I think actually it is not needed.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 11 May 2018 01:50:06 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-10 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#13).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
24 files changed, 665 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/13
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-08 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 12:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc
File src/kudu/hms/hms_catalog-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@237
PS12, Line 237: // Do not add other parameters such as 
'kudu.master_addresses' since
I think it would be better to remove this behavior from the KuduMetastorePlugin 
than work around it here, since the workaround means we might miss test 
coverage of some case where that property applies.


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@456
PS12, Line 456: table.parameters[HmsClient::kLegacyKuduTableNameKey]
instead of calling GetTable and passing in the parameter, perhaps it's better 
to pass in kManagedTableName and kExternalTableName directly, since that's 
really what should be tested?


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@91
PS12, Line 91:   const std::string& name,
This is the name of the table as stored in the HMS, right?  If so, it might be 
clearer to take the database name and table name separately.


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@96
PS12, Line 96: s leg
nit: 'is the legacy...'


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100
PS12, Line 100:   Status RetrieveTables(const char* storage_handler,
I'm not quite following this bit - why is storage_handler taken as an argument 
if the method only does anything when the legacy storage handler is specified?  
Why not just a 'RetrieveLegacyTables' method?  And at that point you don't 
really need a map for the output type, it can just return a vector.


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc@193
PS12, Line 193: on lega
nit: 'non-legacy'


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@67
PS12, Line 67: non Impala
'non-Impala' here and below (non should usually be hyphen separated from the 
word it's modifying)


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@136
PS12, Line 136:   if (!hms::HmsCatalog::IsEnabled()) {
This should probably go before creating the HmsCatalog.


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@145
PS12, Line 145:   ReplicaController::SetVisibility(, 
ReplicaController::Visibility::ALL);
Maybe add a comment here?  I'm not sure why this would need to have non-default 
replica visibility.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 08 May 2018 23:09:27 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-07 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/mini-cluster/external_mini_cluster.h@140
PS12, Line 140:   // If true, set up a Hive Metastore as part of this 
ExternalMiniCluster.
  :   //
  :   // Default: false.
  :   bool enable_hive_metastore;
  :
  :   // If true, set up a Hive Metastore and enable Kudu Hive 
Metastore integration.
  :   //
  :   // Default: false.
  :   bool enable_metastore_integration;
> Why do we need both of these? It seems like they overlap; perhaps this can
Hmm, good point. Will update to a tri-state enum.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 07 May 2018 18:39:38 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-04 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 12:

(2 comments)

Just passing through...

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/mini-cluster/external_mini_cluster.h@140
PS12, Line 140:   // If true, set up a Hive Metastore as part of this 
ExternalMiniCluster.
  :   //
  :   // Default: false.
  :   bool enable_hive_metastore;
  :
  :   // If true, set up a Hive Metastore and enable Kudu Hive 
Metastore integration.
  :   //
  :   // Default: false.
  :   bool enable_metastore_integration;
Why do we need both of these? It seems like they overlap; perhaps this can be 
converted into a tri-state enum class instead?


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto@43
PS12, Line 43:   // Whether or not to create a Hive Metastore.
 :   optional bool enable_hive_metastore = 7;
 :
 :   // Whether or not to create a Hive Metastore and enable the 
HMS integration.
 :   optional bool enable_metastore_integration = 9;
Same feedback.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 04 May 2018 18:22:48 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-03 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#12).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 653 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/12
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-05-03 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#11).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 653 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/11
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-26 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@92
PS6, Line 92:
> I think it is hard to determine whether the tables are legacy or not by nam
Discussed offline with Dan. It looks like it is not safe to have assumption 
like that. We may need to have a flag in the table to identify if it is legacy.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 26 Apr 2018 21:27:46 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-26 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#10).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 654 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/10
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-26 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#9).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 656 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/9
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-26 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#8).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 656 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/8
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-25 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@9
PS6, Line 9: a
> nit: an
Done


http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@13
PS6, Line 13:
> nit: expected
Done


http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py
File build-support/iwyu.py:

http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py@86
PS6, Line 86:   "src/kudu/server/webserver.cc",
:   "src/kudu/util/bit-util-test.cc",
> nit: just in case if you haven't looked at that yet, Todd added a dependenc
Done


http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@92
PS6, Line 92: table_name
> What if 'table_name' is already a 'new' one?  I.e., my concern is that this
I think it is hard to determine whether the tables are legacy or not by names. 
Since legacy tables can also contain '.' in the name. I think it is safe to 
assume the admin run this tool as the first thing to do when enabling hms 
integration.


http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@142
PS6, Line 142: URIs
> nit: URIs
Done


http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@188
PS6, Line 188:   .AddRequiredParameter({ kDefaultDatabaseArg, 
kDefaultDatabaseArgDesc })
 :   .AddOptionalParameter("hive_metastore_uris")
 :   .AddOptionalParameter("hive_metastore_sasl_enabled")
 :   .AddOptionalParameter("hive_metastore_retry_count")
 :   .AddOptionalParameter("hive_metastore_send_timeout")
 :   .AddOptionalParameter("hive_metastore_recv_timeout")
> usability nit: does it make sense to remove the 'hive_metastore_' prefix ?
All of these flags are already defined in hms_catalog.cc. Here just reuse these 
flags to avoid double flags.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 26 Apr 2018 05:18:35 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-25 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#7).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces an upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as
expected.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 659 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/7
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-24 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@9
PS6, Line 9: a
nit: an


http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@13
PS6, Line 13: expect
nit: expected


http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py
File build-support/iwyu.py:

http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py@86
PS6, Line 86:   "src/kudu/tools/tool_action_hms.cc",
:   "src/kudu/tools/kudu-tool-test.cc",
nit: just in case if you haven't looked at that yet, Todd added a dependency of 
the IWYU-related targets on related Thrift auto-generated stuff, so this might 
be not necessary anymore.


http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@92
PS6, Line 92: table_name
What if 'table_name' is already a 'new' one?  I.e., my concern is that this 
methods does not look re-enterable, but I might be missing something.  As I 
understand, kudu_client->ListTables() would output not only legacy tables, 
right?


http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@142
PS6, Line 142: Uris
nit: URIs


http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@188
PS6, Line 188:   .AddOptionalParameter("hive_metastore_uris")
 :   .AddOptionalParameter("hive_metastore_sasl_enabled")
 :   .AddOptionalParameter("hive_metastore_retry_count")
 :   .AddOptionalParameter("hive_metastore_send_timeout")
 :   .AddOptionalParameter("hive_metastore_recv_timeout")
 :   .AddOptionalParameter("hive_metastore_conn_timeout")
usability nit: does it make sense to remove the 'hive_metastore_' prefix ?  If 
the sub-command is 'hms', doesn't it imply that all those options are about 
Hive metastore?

If not, then maybe at least replace 'hive_metastore_' with 'hms_'?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 24 Apr 2018 20:57:44 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-22 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 6: Verified+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 23 Apr 2018 04:30:03 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-22 Thread Hao Hao (Code Review)

Hao Hao has removed a vote on this change.

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-22 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 6:

> Build Failed
 >
 > http://jenkins.kudu.apache.org/job/kudu-gerrit/13067/ : FAILURE

Known flaky test KUDU-1736.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 23 Apr 2018 04:29:58 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-20 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#6).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces a upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as expect.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M build-support/iwyu.py
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
23 files changed, 647 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/6
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-20 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 5:

(22 comments)

http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
File 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java:

http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@61
PS4, Line 61: LEGACY_KUD
> nit: LEGACY
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@127
PS4, Line 127: // can only be upgraded to the new format.
> This is going to reject any alterations of a legacy Kudu table entry that d
Right, I intentionally put it in this way. Because I think this is the right 
behavior. Once HMS integration is enabled, in Alter table event, the plugin 
should only allow legacy Kudu table to be upgraded to the new format. I will 
add a comment to be more explicitly.


http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@150
PS4, Line 150:
> nit: legacy
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@89
PS4, Line 89:   Status UpgradeLegacyImpalaTable(const std::string& id,
> maybe this should be called something like 'UpgradeLegacyTable'? I think th
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@91
PS4, Line 91:   const Schema& schema) 
WARN_UNUSED_RESULT;
> the ordering of the new methods is flipped WRT the .cc
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@95
PS4, Line 95: the c
> It's not clear what is 'List' means here.
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@96
PS4, Line 96:
> Please doc what this key is.
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@183
PS4, Line 183: Status HmsCatalog::UpgradeLegacyImpalaTable(const string& id,
> Add a unit test for this method.
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@190
PS4, Line 190: RETURN_NOT_OK(ParseTableName(name, _name, 
_name));
> Probably worth adding a 'table_names.clear();' line above this, just to be
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@196
PS4, Line 196:
> What happens if you have tables 'a.foo' and 'b.foo', won't this only retain
Updated to use kudu.table_name which should be unique.


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@216
PS4, Line 216:
> begin status messages with lowercase.  Also maybe reword this to call out t
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/mini-cluster/external_mini_cluster.h@332
PS4, Line 332: SetMetastoreIntegration
> Maybe call it EnableMetastoreIntegration in order to be consistent (here an
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@1903
PS4, Line 1903: const shared_ptr& 
kudu_client,
> alignment
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@1971
PS4, Line 1971:   {
> Would be good to add test cases for a table which already has a period in t
Table name that has a period in the name is not working now. I added a TODO in 
https://gerrit.cloudera.org/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@79. 
Because 
https://github.com/apache/kudu/blob/master/src/kudu/hms/hms_catalog.cc#L509 
checks if there are only two parts split by 'dot'. I think we should upgrade 
that code to allow multiple dots in the table name. How do you think?


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@2007
PS4, Line 2007:  external_table_name, 
master_addr));
> Should the external table entry be validated?
Done


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@73
PS4, Line 73: // tables) to follow the format 'database_name.table_name' in 
table naming in Kudu.
> In general I'm finding it difficult to figure out which kinds of tables are
You mean 'vs in HmsCataglog'? Sure, will add more

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-20 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#5).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces a upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as expect.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M build-support/iwyu.py
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
23 files changed, 646 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/5
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-20 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 4:

(22 comments)

http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
File 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java:

http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@61
PS4, Line 61: DEPRECATED
nit: LEGACY


http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@127
PS4, Line 127: checkKuduProperties(newTable);
This is going to reject any alterations of a legacy Kudu table entry that 
doesn't upgrade it to the new format.  I _think_ that's OK, but I want to call 
it out just so we make sure to think through the ramifications of that.


http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@150
PS4, Line 150: deprecated
nit: legacy


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@89
PS4, Line 89:   Status AlterImpalaFormattedTable(const std::string& id,
maybe this should be called something like 'UpgradeLegacyTable'? I think that 
may capture the intended usage better.


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@91
PS4, Line 91:const Schema& schema) 
WARN_UNUSED_RESULT;
the ordering of the new methods is flipped WRT the .cc


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@95
PS4, Line 95: sList
It's not clear what is 'List' means here.


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@96
PS4, Line 96: http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@183
PS4, Line 183: Status HmsCatalog::RetrieveTablesList(const char* 
storage_handler,
Add a unit test for this method.


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@190
PS4, Line 190:   RETURN_NOT_OK(client->GetAllTables(database_name, 
_names));
Probably worth adding a 'table_names.clear();' line above this, just to be 
defensive.


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@196
PS4, Line 196:   hms_tables->emplace(table_name, move(hms_table));
What happens if you have tables 'a.foo' and 'b.foo', won't this only retain one 
of those?


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@216
PS4, Line 216: A
begin status messages with lowercase.  Also maybe reword this to call out the 
error that occcurred (right now it sounds like an action that will be taken).


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/mini-cluster/external_mini_cluster.h@332
PS4, Line 332: SetMetastoreIntegration
Maybe call it EnableMetastoreIntegration in order to be consistent (here and 
below).


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@1903
PS4, Line 1903:   const shared_ptr& 
kudu_client,
alignment


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@1971
PS4, Line 1971:   // 3. Create a non-impala Kudu table.
Would be good to add test cases for a table which already has a period in the 
name (foo.bar), and a table which has an invalid hive character (foo!).


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@2007
PS4, Line 2007: NO_FATALS(ValidateHmsEntries(_client, kudu_client, 
default_database_name,
Should the external table entry be validated?


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@73
PS4, Line 73: Status AlterLegacyKuduTables(const 
client::sp::shared_ptr& kudu_client,
In general I'm finding it difficult to figure out which kinds of tables are 
being handled here vs in HmsUpgrade itself.  Maybe more documentation or 
examples would help.


http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@85
PS4, Line 85: // Renaming existing Impala-managed table name to drop the 
‘impala::’ prefix
Shouldn't this case have already been handled in step 2. below?

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-19 Thread Hao Hao (Code Review)

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#4).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces a upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as expect.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M build-support/iwyu/iwyu-filter.awk
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/schema.h
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
22 files changed, 582 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/4
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-19 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1854
PS1, Line 1854:   table.tableName = table_name;
> Yeah, I have changed the metadata checking in AlterTable event. But I do no
ah ok, I misunderstood this.  I figured the checking would apply the same to 
both.


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@94
PS1, Line 94: }
> Not sure how to fill in default database name based on the existing table 
> name?

That's true.  I think we should just ask the user in that case.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:56:26 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-19 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 3:

> Patch Set 3:
>
> > > Build Failed
>  > >
>  > > http://jenkins.kudu.apache.org/job/kudu-gerrit/13030/ : FAILURE
>  >
>  > Not sure why the jenkin's IWYU build gave errors and the output
>  > does not quite make sense. I built the same patch on a Linux
>  > machine and IWYU didn't complain anything.
>
> The reason might be some pollution in the Jenkins workspaces -- recently 
> there was a patch switching to a new LLVM and IWYU.  So, it might happen that 
> the IWYU build happened at some Jenkins slave with older LLVM/IWYU binary, 
> while at your build machine you saw output from the newest LLVM/IWYU.

Alexey, this has been a consistent issue with any patch touching HMS/thrift 
code.  See https://gerrit.cloudera.org/c/10102/ for an example.

Hao, add the new files to the iwyu-filter in order to make forward progress.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:51:01 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-19 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 3:

> > Build Failed
 > >
 > > http://jenkins.kudu.apache.org/job/kudu-gerrit/13030/ : FAILURE
 >
 > Not sure why the jenkin's IWYU build gave errors and the output
 > does not quite make sense. I built the same patch on a Linux
 > machine and IWYU didn't complain anything.

The reason might be some pollution in the Jenkins workspaces -- recently there 
was a patch switching to a new LLVM and IWYU.  So, it might happen that the 
IWYU build happened at some Jenkins slave with older LLVM/IWYU binary, while at 
your build machine you saw output from the newest LLVM/IWYU.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:48:00 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-19 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 3:

> Build Failed
 >
 > http://jenkins.kudu.apache.org/job/kudu-gerrit/13030/ : FAILURE

Not sure why the jenkin's IWYU build gave errors and the output does not quite 
make sense. I built the same patch on a Linux machine and IWYU didn't complain 
anything.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Apr 2018 18:15:33 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-18 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/hms/hms_client.h@90
PS2, Line 90:   static const char* const kLegacyKuduStorageHandler;
> +1 on legacy over deprecated, maybe extend that to all the old key names?
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1854
PS1, Line 1854:   table.tableName = table_name;
> Isn't that metadata checking going to be problematic in practice?  If it's
Yeah, I have changed the metadata checking in AlterTable event. But I do not 
think it is good to take out the metadata checking in CreateTable event, since 
legacy tables should not be created anymore once HMS integration is on. Or I am 
missing something?


http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/kudu-tool-test.cc@1897
PS2, Line 1897: void ValidateHmsEntries(HmsClient* hms_client,
> I think it'd be simpler to use ASSERTs internally here, and just call it wi
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@94
PS1, Line 94: }
> Yeah, I think in any case where we're going to rename a table it would be b
Sorry, I do not quite follow the last sentence. Not sure how to fill in default 
database name based on the existing table name? I added a param for users to 
specify default database name in the tool, is that what you mean 'kudu table 
name table param'?


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@155
PS1, Line 155:   unordered_map hms_tables_map;
> So would this work if the user passes --unlock_experimental_flags?  I think
Looks like this workaround works. Updated the code.


http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/tool_action_hms.cc@38
PS2, Line 38: #include "kudu/hms/hms_catalog.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Apr 2018 00:50:17 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-18 Thread Hao Hao (Code Review)

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#3).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces a upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as expect.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
18 files changed, 578 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/3
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-17 Thread Dan Burkert (Code Review)

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

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/hms/hms_client.h@90
PS2, Line 90:   static const char* const kDeprecatedKuduStorageHandler;
+1 on legacy over deprecated, maybe extend that to all the old key names?


http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/kudu-tool-test.cc@1897
PS2, Line 1897: Status ValidateHmsEntries(HmsClient* hms_client,
I think it'd be simpler to use ASSERTs internally here, and just call it with 
NO_FATALS(ValidateHmsEntries(..)).  Returning a Status, plus having a boolean 
out param is confusing.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 17 Apr 2018 19:38:37 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-16 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
..


Patch Set 1:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/10075/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
File 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java:

http://gerrit.cloudera.org:8080/#/c/10075/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@174
PS1, Line 174: // Test altering a Kudu (or a historical) table.
> nit: I think the term 'legacy' is better than historical, because historica
I choose to use 'legacy' here since it is mainly testing that alters table can 
succeed for all kinds of legacy kudu tables (either managed or external table).


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.h@95
PS1, Line 95:   Status RetrieveKuduTablesList(const char* kudu_storage_handler,
> As far as I can tell this isn't specific to Kudu at all, it's a generic loo
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.cc@196
PS1, Line 196:   hms_tables->emplace(move(table_name), move(hms_table));
> warning: std::move of the const variable 'table_name' has no effect; remove
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.cc@206
PS1, Line 206: const string& name,
> warning: parameter 'name' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_client.h@94
PS1, Line 94:   static const char* const kKuduTableNameKey;
> Are we going to be using this going forward?  If not, maybe add 'Deprecated
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/mini-cluster/external_mini_cluster.h@148
PS1, Line 148:   bool enable_metastore_integration;
> Could you tweak this a bit so there is only one flag to flip in the common
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1854
PS1, Line 1854:   // Do not add other parameters such as 
'kudu.master_addresses' since
> This is problematic because today's HMS table entries that impala creates c
Right, but I cannot think of another clean way to get away from the 
KuduMetastorePlugin metadata checking for onCreateTable event. Besides, I do 
not feel it is crucial to add 'kudu.master_addresses' here for testing.


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1870
PS1, Line 1870:  !MatchPattern(table_name,
> Shouldn't this be a strict prefix match?  If so HasPrefixString is probably
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1874
PS1, Line 1874: TEST_F(ToolTest, TestHmsUpgrade) {
> There are three distinct kinds of tables we need to upgrade:
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1888
PS1, Line 1888:   string historical_table_name = 
StrCat(HmsClient::kHistoricalTablePrefix,
> The existing managed table format is 'impala::my_db.my_table'.
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1904
PS1, Line 1904: StrCat(HmsClient::kHistoricalTablePrefix, table_name)
> historical_table_name
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1945
PS1, Line 1945: ASSERT_EQ(database_name, dbname);
> If there is only one database and one table, why the for loops?
Added more test tables as you suggested.


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@29
PS1, Line 29: #include "kudu/gutil/strings/strcat.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@57
PS1, Line 57: using std::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@64
PS1, Line 64: const char* const kMasterAddressesArg = "master_addresses";
> This is defined in tool_action_common.h.
Done

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-16 Thread Hao Hao (Code Review)

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10075

to look at the new patch set (#2).

Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces a upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as expect.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
18 files changed, 585 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/2
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191: Metadata Upgrade Tool

2018-04-16 Thread Hao Hao (Code Review)

Hao Hao has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10075


Change subject: KUDU-2191: Metadata Upgrade Tool
..

KUDU-2191: Metadata Upgrade Tool

This commit introduces a upgrade tool to allows backward compatibility.
It provides support to upgrade the metadata format of existing Impala
managed/external tables in HMS entries, as well as rename the existing
tables in Kudu to adapt to the new naming rules. It adds test case
using external mini cluster to ensure the upgrade tool works as expect.

Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
---
M 
java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M 
java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
18 files changed, 488 insertions(+), 24 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10075/1
--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao

48 matches

Mail list logo