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 2019/05/13 06:16:59 UTC
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13317
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
---
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
2 files changed, 27 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13317/1
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13317/2/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/13317/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@137
PS2, Line 137: // Allow non-Kudu tables to be altered, but ensure that the alteration
: // isn't introducing Kudu-specific properties. This includes legacy tables,
: // however excludes upgrading case (altering legacy Kudu table to be Kudu table).
: if (!isKuduTable(oldTable) &&
: !(isLegacyKuduTable(oldTable) && isKuduTable(newTable))) {
: checkNoKuduProperties(newTable);
: return;
: }
I'm finding this difficult to reason about without enumerating every case.
Would something like this be sufficient:
if (!isKuduTable(oldTable) && !isLegacyKuduTable(oldTable) &&
!isKuduTable(newTable) && !isLegacyKuduTable(newTable)) {
// This isn't a Kudu table.
checkNoKuduProperties(newTable);
return;
}
if (isLegacyKuduTable(oldTable) && isLegacyKuduTable(newTable)) {
// We're altering a legacy table and therefore don't care (but with an actual explanation).
return;
}
if (isKuduTable(oldTable) && isLegacyKuduTable(newTable)) {
checkKuduProperties(newTable);
return;
}
if (isKuduTable(newTable)) {
checkKuduProperties(newTable);
}
...
I'd prefer this style because it makes reasoning about oldTable and newTable very straightforward IMO. As you read through the code, invariants about the state of the alter operation are known, rather than the states of each table (which I think are harder to reason about). This isn't a full implementation, but does that style make sense? If so, could you update this function to use it?
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 14 May 2019 19:17:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13317
to look at the new patch set (#4).
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
---
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
2 files changed, 93 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13317/4
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13317/5/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/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@141
PS5, Line 141: // potential schema alterations are coming from the Kudu master
nit: add period
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@145
PS5, Line 145: } else {
nit: the 'else' clause is not needed because of 'return' above ?
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@154
PS5, Line 154: // schema alterations are coming from the Kudu master
nit: add period
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@155
PS5, Line 155: checkNoKuduProperties(newTable);
I don't have enough context on this, but this looks a bit strange: no Kudu properties in legacy Kudu table. Is it correct? Do we have a test scenario for this at least?
http://gerrit.cloudera.org:8080/#/c/13317/5/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/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@258
PS5, Line 258: // Check that altering table with Kudu storage handler to legacy format
: // succeeds.
: {
: Table alteredTable = table.deepCopy();
: alteredTable.getParameters().clear();
: alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
: KuduMetastorePlugin.LEGACY_KUDU_STORAGE_HANDLER);
: alteredTable.putToParameters(KuduMetastorePlugin.LEGACY_KUDU_TABLE_NAME,
: "legacy_table");
: alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
: "localhost");
: client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
: }
I thought that after recent developments this should no longer be a case, no? If such, should we modify the code of the handler and add a negative test scenario instead?
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@331
PS5, Line 331: table
nit: table's
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@338
PS5, Line 338: table
nit: table's
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 17 May 2019 03:06:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13317/5/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/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@258
PS5, Line 258: // Check that altering table with Kudu storage handler to legacy format
: // succeeds.
: {
: Table alteredTable = table.deepCopy();
: alteredTable.getParameters().clear();
: alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
: KuduMetastorePlugin.LEGACY_KUDU_STORAGE_HANDLER);
: alteredTable.putToParameters(KuduMetastorePlugin.LEGACY_KUDU_TABLE_NAME,
: "legacy_table");
: alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
: "localhost");
: client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
: }
> Even with recent development which is to always use new storage handler goi
In the case a user needs to downgrade to <Impala 3.3 they will also need to disable HMS integration which will disable this plugin.
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 20 May 2019 13:28:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13317
to look at the new patch set (#2).
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
---
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
2 files changed, 44 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13317/2
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13317/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/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@144
PS4, Line 144: } else {
: // Allow legacy tables to be altered without introducing Kudu-specific
: // properties.
: checkNoKuduProperties(newTable);
: }
> nit: for parity with the L156, could write this as
Done
http://gerrit.cloudera.org:8080/#/c/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@158
PS4, Line 158: properties. And the
: // schema alteration is not coming through Hive, if any.
> nit: reword a bit:
Done
http://gerrit.cloudera.org:8080/#/c/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@240
PS4, Line 240: * Checks that the Kudu table schema should not be altered through Hive.
> An Impala user might be the one updating the HMS metadata though, no? Would
Done
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 16 May 2019 21:07:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13317/1/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/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@49
PS1, Line 49: * The plugin enforces that Kudu table entries in the HMS always contain
: * two properties: a Kudu table ID and the Kudu master addresses. It also
: * enforces that non-Kudu tables do not have these properties (except cases
: * when upgrading tables with legacy Kudu storage handler to be Kudu tables
: * or downgrading from the other way around). The plugin considers entries
: * to be Kudu tables if they contain the Kudu storage handler.
: *
: * Additionally, the plugin checks that when particular events have an
: * environment containing a Kudu table ID, that event only applies
> Can you update this to describe how this plugin handles the legacy storage
Done
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@133
PS1, Line 133:
> This should probably be updated. Without further clarification, the new cod
Done
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@143
PS1, Line 143: return;
> Why isn't this using isKuduTable()? Why !isLegacyKuduTable()? Does it matte
It is important to prevent cases alter Kudu table to be without Kudu storage handler. However for the below it should be equivalent, as there is only KuduTable or LegacyTable at that point.
http://gerrit.cloudera.org:8080/#/c/13317/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/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@25
PS1, Line 25: import java.util.UUID;
:
> Not used?
Done
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@326
PS1, Line 326: cy Kudu table without context succeeds.
> nit: perhaps less confusing as alteredTabled.getDbName() and alteredTable.g
Not quite follow this. I think here it is intend to use the original table name.
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@329
PS1, Line 329:
> nit: renaming
Done
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@337
PS1, Line 337: // Check that renaming legacy table schema succeeds.
> Should this be dropping the renamed table?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 14 May 2019 00:55:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed Tony Foerster from this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Removed reviewer Tony Foerster.
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13317/2/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/13317/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@137
PS2, Line 137: // Allow legacy tables to be altered without introducing Kudu-specific
: // properties.
: if (isLegacyKuduTable(oldTable) && isLegacyKuduTable(newTable)) {
: checkNoKuduProperties(newTable);
: return;
: }
:
: /
> I'm finding this difficult to reason about without enumerating every case.
Thanks a lot for the proposal! The style makes sense, and I refactored the code to be cleaner.
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 14 May 2019 23:21:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13317
to look at the new patch set (#6).
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
---
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
2 files changed, 91 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13317/6
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 May 2019 15:32:13 +0000
Gerrit-HasComments: No
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13317/5/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/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@258
PS5, Line 258: // Check that altering table with Kudu storage handler to legacy format
: // succeeds.
: {
: Table alteredTable = table.deepCopy();
: alteredTable.getParameters().clear();
: alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
: KuduMetastorePlugin.LEGACY_KUDU_STORAGE_HANDLER);
: alteredTable.putToParameters(KuduMetastorePlugin.LEGACY_KUDU_TABLE_NAME,
: "legacy_table");
: alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
: "localhost");
: client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
: }
> In the case a user needs to downgrade to <Impala 3.3 they will also need to
I don't think we actually need to address it here. The main motivation of this patch is to allow altering legacy tables (without downgrade or upgrade) when the plugin is used. If we decide we don't want to support downgrade with the plugin enabled, we can do it in a follow up change.
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 May 2019 06:40:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13317
to look at the new patch set (#5).
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
---
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
2 files changed, 93 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13317/5
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 22 May 2019 01:30:12 +0000
Gerrit-HasComments: No
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Reviewed-on: http://gerrit.cloudera.org:8080/13317
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
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
2 files changed, 91 insertions(+), 40 deletions(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13317/1/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/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@49
PS1, Line 49: * The plugin enforces that Kudu table entries in the HMS always
: * contain two properties: a Kudu table ID and the Kudu master addresses. It also
: * enforces that non-Kudu tables do not have these properties. The plugin
: * considers entries to be Kudu tables if they contain the Kudu storage handler.
: *
: * Additionally, the plugin checks that when particular events have an
: * environment containing a Kudu table ID, that event only applies
: * to the specified Kudu table. This provides some amount of concurrency safety,
: * so that the Kudu Master can ensure it is operating on the correct table entry.
Can you update this to describe how this plugin handles the legacy storage handler?
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@133
PS1, Line 133: // Allow non-Kudu tables (even the legacy ones) to be altered.
This should probably be updated. Without further clarification, the new code makes everything below L138 more difficult to reason about IMO. Previously, after L138 we were guaranteed that the old table was either a Kudu table or a Legacy table. Now it's less obvious what guarantees there are.
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@143
PS1, Line 143: if (!isLegacyKuduTable(newTable)) {
Why isn't this using isKuduTable()? Why !isLegacyKuduTable()? Does it matter? Same below.
http://gerrit.cloudera.org:8080/#/c/13317/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/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@25
PS1, Line 25: import java.util.ArrayList;
: import java.util.List;
Not used?
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@326
PS1, Line 326: table.getDbName(), table.getTableName()
nit: perhaps less confusing as alteredTabled.getDbName() and alteredTable.getTableName()?
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@329
PS1, Line 329: altering
nit: renaming
http://gerrit.cloudera.org:8080/#/c/13317/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@337
PS1, Line 337: client.dropTable(table.getDbName(), table.getTableName());
Should this be dropping the renamed table?
Alternatively, put the above new cases into their own separate test case to test doing things with legacy tables. As it stands, it's kind of unclear to me which test cases are testing legacy metadata. If you do, you could also put L343-L347 in the same test case.
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 13 May 2019 07:13:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13317
to look at the new patch set (#3).
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Allow alter legacy tables in Hive Metastore Kudu plugin
Currently, the Hive metastore plugin disallows schema alteration for
tables with legacy storage handler. Since the plugin is intended to
work with both before and post Kudu/HMS integration context, this patch
ensures creation/alteration/deletion on legacy tables works as expected.
Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
---
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
2 files changed, 85 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/13317/3
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 4:
(3 comments)
Thanks for the double-refactor; just some nits at this point.
http://gerrit.cloudera.org:8080/#/c/13317/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/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@144
PS4, Line 144: } else {
: // Allow legacy tables to be altered without introducing Kudu-specific
: // properties.
: checkNoKuduProperties(newTable);
: }
nit: for parity with the L156, could write this as
...
checkNoAlterKuduSchema(tableEvent, oldTable, newTable);
return;
}
checkNoKuduProperties(newTable);
}
or change L156 to use an `else`
http://gerrit.cloudera.org:8080/#/c/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@158
PS4, Line 158: properties. And the
: // schema alteration is not coming through Hive, if any.
nit: reword a bit:
"properties, and that any potential schema alterations are coming from the Kudu master."
Same above in both comments.
http://gerrit.cloudera.org:8080/#/c/13317/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@240
PS4, Line 240: * Checks that the Kudu table schema should not be altered through Hive.
An Impala user might be the one updating the HMS metadata though, no? Would this be more accurate as:
"Checks that the table schema can only be altered by an action from the Kudu Master."
? If so, perhaps update the exception text so that's clear as well.
By the same token, would something like checkOnlyKuduMasterCanAlterSchema be an apt name?
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 15 May 2019 07:40:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 3: Verified+1
Unrelated flaky test failure.
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 15 May 2019 00:35:30 +0000
Gerrit-HasComments: No
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 May 2019 18:57:48 +0000
Gerrit-HasComments: No
[kudu-CR] Allow alter legacy tables in Hive Metastore Kudu plugin
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin
......................................................................
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13317/5/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/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@141
PS5, Line 141: // potential schema alterations are coming from the Kudu master
> nit: add period
Done
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@145
PS5, Line 145: } else {
> nit: the 'else' clause is not needed because of 'return' above ?
Done
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@154
PS5, Line 154: // schema alterations are coming from the Kudu master
> nit: add period
Done
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@155
PS5, Line 155: checkNoKuduProperties(newTable);
> I don't have enough context on this, but this looks a bit strange: no Kudu
Yeah, this is correct. Because checkNoKuduProperties verify that legacy tables don't use the new storage handler and don't contain table_ID property. We have test at L258 at TestKuduMetastorePlugin.
http://gerrit.cloudera.org:8080/#/c/13317/5/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/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@258
PS5, Line 258: // Check that altering table with Kudu storage handler to legacy format
: // succeeds.
: {
: Table alteredTable = table.deepCopy();
: alteredTable.getParameters().clear();
: alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
: KuduMetastorePlugin.LEGACY_KUDU_STORAGE_HANDLER);
: alteredTable.putToParameters(KuduMetastorePlugin.LEGACY_KUDU_TABLE_NAME,
: "legacy_table");
: alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
: "localhost");
: client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
: }
> I thought that after recent developments this should no longer be a case, n
Even with recent development which is to always use new storage handler going forward (>= Kudu 1.10 and >= Impala 3.3). We still should allow table to be downgraded to use legacy storage handlers, for cases when users want to downgrade to older versions( <Impala 3.3).
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@331
PS5, Line 331: table
> nit: table's
Done
http://gerrit.cloudera.org:8080/#/c/13317/5/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@338
PS5, Line 338: table
> nit: table's
Done
--
To view, visit http://gerrit.cloudera.org:8080/13317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3
Gerrit-Change-Number: 13317
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 17 May 2019 22:57:28 +0000
Gerrit-HasComments: Yes