You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2018/06/29 21:25:16 UTC

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata 

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


Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 7 insertions(+), 4 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 6:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10844/5/src/kudu/tools/tool_action_hms.cc@166
PS5, Line 166: Prepen
> nit: we generally use CloneAndPrepend() a la RETURN_NOT_OK_PREPEND().
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Jul 2018 22:06:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10844/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10844/4//COMMIT_MSG@9
PS4, Line 9: TestHmsDowngrade keeps on failing as the metadata
> Have you looped the updated test via dist-test?
I didn't because I think this is a broken test but rather a flaky test. But did one with the latest patch: http://dist-test.cloudera.org/job?job_id=hao.hao.1530566672.124129. TestHmsDowngrade passed 500 out of 500.


http://gerrit.cloudera.org:8080/#/c/10844/4//COMMIT_MSG@10
PS4, Line 10: This issue
            : didn't surface because commit 214dbc249 didn't rebase. This commit
            : corrects the upgrade tool to better handle this scenario.
> To clarify, this means there's some logic in some other commit that was inc
Yeah, 214dbc249 didn't rebase with latest change (which includes the change in commit 985c6698a) before push so this issue didn't pop up.


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

http://gerrit.cloudera.org:8080/#/c/10844/4/src/kudu/tools/tool_action_hms.cc@170
PS4, Line 170: 
             :         }
             :         if (s.ok()) {
             :           s = hms_catalog->UpgradeLegacyImpalaTable(
> Do you think this message would be helpful to append to add to the Status i
Done


http://gerrit.cloudera.org:8080/#/c/10844/4/src/kudu/tools/tool_action_hms.cc@176
PS4, Line 176: 
             :       }
             :     } else {
> This usage of AndThen feels a bit weird. Generally, I think of it as chaini
Makes sense, will update. Thanks!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Jul 2018 21:39:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10844/3/src/kudu/tools/tool_action_hms.cc@164
PS3, Line 164:           s = hms_catalog->UpgradeLegacyImpalaTable(
> I think line 169 is supposed to be in this if block?  As is there can be tw
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 30 Jun 2018 00:55:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/10844/2/src/kudu/tools/tool_action_hms.cc@163
PS2, Line 163:  In this case, we only upgrade the legacy Impala table.
IIUC the previous statement is unconditionally renaming the kudu table, should the rename be behind a table_name != new_table_name check?


http://gerrit.cloudera.org:8080/#/c/10844/2/src/kudu/tools/tool_action_hms.cc@169
PS2, Line 169: s.ok() || table_name == new_table_name
What if s is a network error or something else?  I don't think you'd want to edit the HMS in that case.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 30 Jun 2018 00:10:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/10844/2/src/kudu/tools/tool_action_hms.cc@163
PS2, Line 163: ew_table_name) {
> IIUC the previous statement is unconditionally renaming the kudu table, sho
Yeah, I agree. Was trying to reduce the number of the call. But I think as you pointed out, it does not apply here.


http://gerrit.cloudera.org:8080/#/c/10844/2/src/kudu/tools/tool_action_hms.cc@169
PS2, Line 169: duTableOnly(kudu_client, table_name, n
> What if s is a network error or something else?  I don't think you'd want t
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 30 Jun 2018 00:22:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, 

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

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

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

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

I also looped this test for 500 times, no failures found for
TestHmsDowngrade. Dist-test:
http://dist-test.cloudera.org/job?job_id=hao.hao.1530566672.124129

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 15 insertions(+), 14 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/10844/1/src/kudu/tools/tool_action_hms.cc@171
PS1, Line 171:           // Only upgrade legacy impala table if the Kudu table with hive-compatible
This comment is confusing, because the legacy impala table is upgraded on line 166.  Would it be possible to only call the method once by if-gating the AlterKuduTableOnly call?


http://gerrit.cloudera.org:8080/#/c/10844/1/src/kudu/tools/tool_action_hms.cc@171
PS1, Line 171: impala table if the Kudu table with hive
uppercase Impala and Hive



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Jun 2018 21:50:18 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Jul 2018 22:24:08 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 5: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10844/5/src/kudu/tools/tool_action_hms.cc@166
PS5, Line 166: Append
nit: we generally use CloneAndPrepend() a la RETURN_NOT_OK_PREPEND().



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Jul 2018 21:48:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10844/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10844/4//COMMIT_MSG@9
PS4, Line 9: TestHmsDowngrade keeps on failing as the metadata
Have you looped the updated test via dist-test?


http://gerrit.cloudera.org:8080/#/c/10844/4//COMMIT_MSG@10
PS4, Line 10: This issue
            : didn't surface because commit 214dbc249 didn't rebase. This commit
            : corrects the upgrade tool to better handle this scenario.
To clarify, this means there's some logic in some other commit that was incorrectly rebased?


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

http://gerrit.cloudera.org:8080/#/c/10844/4/src/kudu/tools/tool_action_hms.cc@170
PS4, Line 170: LOG(WARNING) << Substitute("Failed to upgrade legacy Impala table '$0.$1' "
             :                                        "(Kudu table name: $2), because a Kudu table with "
             :                                        "name '$0.$1' already exists'.", hms_table->dbName,
             :                                        hms_table->tableName, table_name);
Do you think this message would be helpful to append to add to the Status itself? Eg through CloneAndPrepend or something?


http://gerrit.cloudera.org:8080/#/c/10844/4/src/kudu/tools/tool_action_hms.cc@176
PS4, Line 176: return hms_catalog->UpgradeLegacyImpalaTable(
             :                       kudu_table->id(), hms_table->dbName, hms_table->tableName,
             :                       client::SchemaFromKuduSchema(kudu_table->schema()));
This usage of AndThen feels a bit weird. Generally, I think of it as chaining together calls, but in this case, we're using it pretty much exclusively for control-flow, which IMO feels kind of unnatural. Since this is being called both here and at L164, maybe we can combine the usages? How about something like:

 if (table_name != new_table_name) {
   s = AlterKuduTableOnly();
   if (s.IsAlreadyPresent()) LOG(WARNING) << error;
 }
 if (s.ok()) {
   s = hms_catalog->UpgradeLegacyImpalaTable();
 }



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Jul 2018 18:23:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10844/1/src/kudu/tools/tool_action_hms.cc@163
PS1, Line 163:         if (!exist) {
             :           // TODO(Hao): Use notification listener to avoid race conditions.
             :           s = AlterKuduTableOnly(kudu_client, table_name, new_table_name).AndThen([&] {
             :             return hms_catalog->UpgradeLegacyImpalaTable(kudu_table->id(),
             :                 hms_table->dbName, hms_table->tableName,
             :                 client::SchemaFromKuduSchema(kudu_table->schema()));
             :           });
             :         } else {
             :           // Only upgrade legacy impala table if the Kudu table with hive-compatible
             :           // name already exists.
             :           RETURN_NOT_OK(hms_catalog->UpgradeLegacyImpalaTable(
             :               kudu_table->id(),
             :               hms_table->dbName,
             :               hms_table->tableName,
             :               client::SchemaFromKuduSchema(kudu_table->schema())));
             :         }
Can this be rewritten? Looks like the calls to UpgradeLegacyImpalaTable are exactly the same in both branches.

Also, why is UpgradeLegacyImpalaTable non-fatal (i.e. just stores the result into 's') in the !exist case, but fatal (via RETURN_NOT_OK) in the exist case?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Jun 2018 21:43:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

I also looped this test for 500 times, no failures found for
TestHmsDowngrade. Dist-test:
http://dist-test.cloudera.org/job?job_id=hao.hao.1530566672.124129

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Reviewed-on: http://gerrit.cloudera.org:8080/10844
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 15 insertions(+), 14 deletions(-)

Approvals:
  Andrew Wong: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, 

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

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

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

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 16 insertions(+), 11 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10844/3/src/kudu/tools/tool_action_hms.cc@164
PS3, Line 164:           s = hms_catalog->UpgradeLegacyImpalaTable(
I think line 169 is supposed to be in this if block?  As is there can be two calls to UpgradeLegacyImpalaTable



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 30 Jun 2018 00:35:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/10844/1/src/kudu/tools/tool_action_hms.cc@171
PS1, Line 171: ), hms_table->dbName, hms_table->tableNa
> uppercase Impala and Hive
Done


http://gerrit.cloudera.org:8080/#/c/10844/1/src/kudu/tools/tool_action_hms.cc@171
PS1, Line 171:                   kudu_table->id(), hms_table->dbName, hms_table->tableName,
> This comment is confusing, because the legacy impala table is upgraded on l
Done


http://gerrit.cloudera.org:8080/#/c/10844/1/src/kudu/tools/tool_action_hms.cc@163
PS1, Line 163:         // then downgraded. In this case, we only upgrade the legacy Impala table.
             :         if (s.IsAlreadyPresent() && table_name != new_table_name) {
             :           LOG(WARNING) << Substitute("Failed to upgrade legacy Impala table '$0.$1' "
             :                                      "(Kudu table name: $2), because a Kudu table with "
             :                                      "name '$0.$1' already exists'.", hms_table->dbName,
             :                                      hms_table->tableName, table_name);
             :         } else if (s.ok() || table_name == new_table_name) {
             :           s = hms_catalog->UpgradeLegacyImpalaTable(
             :                   kudu_table->id(), hms_table->dbName, hms_table->tableName,
             :                   client::SchemaFromKuduSchema(kudu_table->schema()));
             :         }
             :       }
             :     } else {
             :       // Create the table in the HMS.
             :       string new_table_name = Substitute("$0.$1", default_database, table_name);
             :       Sch
> Can this be rewritten? Looks like the calls to UpgradeLegacyImpalaTable are
Realized the logic is not quite right in this patch as there might be another different tables with same name already exist. So updated it, And you are right, this should also be stored.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Jun 2018 23:47:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10844 )

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Jul 2018 21:42:41 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, 

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

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

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

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 18 insertions(+), 13 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, 

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

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

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

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 9 insertions(+), 11 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, 

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

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

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

Change subject: KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata
......................................................................

KUDU-2191: Adjust upgrade tool to tolerate downgraded metadata

Currently, TestHmsDowngrade keeps on failing as the metadata upgrade
tool does not handle tables being downgraded properly. This issue
didn't surface because commit 214dbc249 didn't rebase. This commit
corrects the upgrade tool to better handle this scenario.

I also looped this test for 500 times, no failures found for
TestHmsDowngrade. Dist-test:
http://dist-test.cloudera.org/job?job_id=hao.hao.1530566672.124129

Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
---
M src/kudu/tools/tool_action_hms.cc
1 file changed, 15 insertions(+), 14 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9a3b154e77db4eda72b9c4b53861f9fe06f09a3
Gerrit-Change-Number: 10844
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins