[kudu-CR] KUDU-2191: Metadata Upgrade Tool
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
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
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
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
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
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
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
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
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
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
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
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, &exist); 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
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
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
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
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
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
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
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(&builder, 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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, &database_name, &table_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,
[kudu-CR] KUDU-2191: Metadata Upgrade Tool
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
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, &table_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(&hms_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? http://gerrit.cloudera.org:8080/#/c
[kudu-CR] KUDU-2191: Metadata Upgrade Tool
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
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
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
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
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
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
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
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
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 ) Change subject: KUDU-2191: Metadata Upgrade Tool .. Patch Set 2: (3 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; Isn't that metadata checking going to be problematic in practice? If it's checking that legacy tables don't have the kudu.master_addresses property and failing to roll them forward if so, then we aren't going to be able to upgrade legacy tables, right? > I do not feel it is crucial to add 'kudu.master_addresses' here for testing. I think it's important to test against entries which are as similar to what we'll be encountering in real clusters as possible. 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: // TODO(Hao): Do we need to check if Impala-managed table is not present in HMS? > Good point. Maybe we should instead ask the users to input which database h Yeah, I think in any case where we're going to rename a table it would be better to confirm first, and if we can, we should fill in a default based on either the existing table name or kudu table name table param. http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@155 PS1, Line 155: ReplicaController::SetVisibility(&builder, ReplicaController::Visibility::ALL); > Right, experimental flags are checked and have to unlock them with FLAGS_un So would this work if the user passes --unlock_experimental_flags? I think that will be preferred in the short-term, rather than having two different flag names. In the long term we'll be making these flags stable. -- 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 17:44:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191: Metadata Upgrade Tool
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 http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_a
[kudu-CR] KUDU-2191: Metadata Upgrade Tool
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
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 ) Change subject: KUDU-2191: Metadata Upgrade Tool .. Patch Set 1: (19 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 historical can mean a bunch of different things. But even better would be to be specific with what type of table this is, e.g. an Impala managed table or Impala 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 lookup for any table with the provided storage handler. 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' to the name. 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 case of wanting a mini HMS plus wanting the mini kudu cluster to have the integration enabled? I think the case of wanting a mini HMS but not wanting the integration enabled will be very rare. One way that could be done is have this new flag imply enable_hive_metastore, and switch all call sites just to use the new one. 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 can have the kudu.master_addresses property. Here's a sample: parameters={ STATS_GENERATED: TASK, kudu.master_addresses: nightly6x-1.gce.cloudera.com, kudu.table_name: impala::default.managed_table, numRows: 80241, storage_handler: com.cloudera.kudu.hive.KuduStorageHandler, transient_lastDdlTime: 1523648851 }, 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 more straightforward. 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: * managed impala table * external impala table * non-impala Kudu table which doesn't have an HMS entry This test should differentiate these cases and ensure each works. 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'. 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 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? 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@64 PS1, Line 64: const char* const kMasterAddressesArg = "master_addresses"; This is defined in tool_action_common.h. http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@66 PS1, Line 66: const char* const kHmsUrisArg = "hms_uris"; Could you use the existing hive_metastore_uris flag to be consistent? http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@69 PS1, Line 69: bool IsHistoricalTable(const string& table_name) { Doc this? Kudu doesn't currently have any restrictions
[kudu-CR] KUDU-2191: Metadata Upgrade Tool
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