You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org> on 2019/10/09 01:08:42 UTC

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14397

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

In HMS 3.1 there is a default tranformer introduced which checks the client capabilities
and transforms a table before creating it. Additionally, it also makes sure that any
managed table which is created is transactional. If a user creates a managed table which
is not transactional, it automatically converts such table as external and sets certain
table properties to mark such transformed tables.

This presents a problem for managed Kudu tables in Impala since managed and external
tables are handled differently in Kudu. Specifically, if a Kudu table is managed, certain
operations like drop table, rename table, alter table are performed on the Kudu side along
with updating the catalog. If the Kudu table is external, the Kudu operations are skipped
and only catalog side operations are performed.

When the user creates a managed Kudu table, user expects that drop table, rename table
should be updated by Impala automatically in Kudu as well. But since HMS 3 transforms such
managed tables into external, currently Impala does not perform the Kudu side operations
breaking the semantics for the user.

This patch makes changes to Catalog so that it can detect such transformed external tables
and perform Kudu side operations similar to what it was doing for managed Kudu table when
talking with previous HMS versions.

Testing:
1. Ran existing Kudu tests which were failing when the CDP build number was bumped up.
2. Running all tests with USE_CDP_HIVE flag set to true.

Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
6 files changed, 38 insertions(+), 25 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/1
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

Hao Hao has uploaded a new patch set (#10) to the change originally created by Vihang Karajgaonkar. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Testing:
1. Bumped up the CDP build number in a private build so that the HMS
translation logic is pulled in. Ran all the tests. Without the patch there are many Kudu
tests which were failing. After the patch none of the Kudu tests fail. There were
additional Ranger tests which failed due to the CDP bump but those were unrelated to
this patch and should be fixed as part of a separate change when the CDP build number
is bumped up.

Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
8 files changed, 105 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/10
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/14397/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@343
PS7, Line 343:     if (Boolean.parseBoolean(getTblProperties().get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE))) {
line too long (96 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 23:13:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4843/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 19:20:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isSynchronizedTable(tabl
> synchronized comes from whether a table is kept in-snyc with HMS in Kudu HM
I agree the naming could be useful. I synced up with Hao offline to understand what it means when we say synchronized Kudu tables. I updated the change to rename this method to synchronized table.


http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@142
PS3, Line 142: isExternalTable(msTbl) && !Boolean
             :         .parseBoolean(msTbl.getParameters().get(TBL_PROP_EXTERNAL_TABLE_PURGE));
> This should be reversed now, since a synchronized table is either managed o
yeah thats right. I had noticed it but forgot to publish the patchset which makes the appropriate changes.


http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@151
PS3, Line 151:   public static boolean isExternalTable(org.apache.hadoop.hive.metastore.api.Table msTbl) {
> line too long (91 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Oct 2019 17:26:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/5
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1619
PS1, Line 1619:       if (!KuduTable.isKuduTable(msTable) || KuduTable.isExternallyManagedTable(msTable)) continue;
line too long (99 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Oct 2019 01:09:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4832/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 00:05:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Testing:
1. Bumped up the CDP build number in a private build so that the HMS translation logic is
pulled in. Ran all the tests. Without the patch there are many Kudu tests which were
failing. After the patch none of the Kudu tests fail. There were additional Ranger tests
which failed due to the CDP bump but those were unrelated to this patch and should be
fixed as part of a separate change when the CDP build number is bumped up.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/2
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@151
PS3, Line 151:   public static boolean isExternalTable(org.apache.hadoop.hive.metastore.api.Table msTbl) {
line too long (91 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 21:08:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
> So this is interesting. Note that this method is implemented to create mana
Hmm, it will be translated to 'Preconditions.checkState(KuduTable.isSynchronizedTable(msTbl))' instead of 'Preconditions.checkState(!KuduTable.isSynchronizedTable(msTbl))', right? So only the following should be allowed:
create table foo (id int);
create external table foo (id int) tblproperties ('external.purge.table' = 'false');

There are actually big differences between treating 'create table foo(id int) tblproperties ('external.purge.table'='true');` as external or synchronized:
1) creating external table has to have 'kudu.table_name' property specified. That means user can only do
create external table foo (id int) tblproperties ('external.purge.table' = 'false', 'kudu.table_name' = 'my_kudu_table'); And Kudu table name will be the one specified by the user.
2) creating a synchronized table means a Kudu table will be created by Impala, while creating external table will not.

Therefore, after pondering more, I think we should either block 'external.purge.table'='true' for Kudu tables as it conflicts with the definition of synchronized table. Then we can keep the current logic of checking for external table here. Or treat external + 'external.purge.table'='true' as managed table for table naming, and in analyzing create table statement (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java#L255). And update to check for synchronized table here. I am good with either approach.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 00:20:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.

Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14397/1/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/1/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS1, Line 120:     if (!KuduTable.isExternallyManagedTable(table_.getMetaStoreTable())) {
If we use the purge property as mentioned in my other comment, I think this check should be for managed or "external purge" tables.


http://gerrit.cloudera.org:8080/#/c/14397/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@141
PS1, Line 141:         .parseBoolean(msTbl.getParameters().get(TBL_PROP_TRANSLATED_TO_EXTERNAL));
Instead of using the `TRANSLATED_TO_EXTERNAL` property, I think we should use the `external.table.purge=true` property. That is what was used in the Kudu side changes to support this. See the patch for context here: https://gerrit.cloudera.org/#/c/14179/



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Oct 2019 01:21:46 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.

Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isExternallyManagedTable
> Yeah, I guess I would prefer the naming 'isSynchronizedTable' as it is more
I agree, this name was confusing to me.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 14:54:43 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

(1 comment)

> Patch Set 8:
> 
> (3 comments)
> 
> I think HDFS and HBase tables also have this issue. For example, creating a non-transactional HDFS table will finally create an external table. Then in DROP TABLE we won't drop HDFS files.
> 
> However, let's keep this fix simple and focus on kudu first.

Sorry, only managed Kudu tables are affected. For HDFS tables and HBase tables, Impala never remove underlying files or hbase tables. The removal works are done by Hive so they will always be right.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@140
PS8, Line 140:   public static boolean isSynchronizedTable(
> Could you move these three functions into Table.java? I feel like it's not 
Sorry, don't need to move these... Only Kudu tables are affected.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 15:16:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
> That said, if someone more familiar with Impala metadata feels that option 
I also thinks option 1 is more safe, and as no one so far says it is safe with option 2. I just pushed a patch with option 1.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 23:14:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@141
PS2, Line 141: !Boolean
             :         .parseBoolean
> I know its a bit confusing. When external.table.purge is true, it means tha
I see. I misconstrued "externally managed" for "external but having managed semantics (i.e. purge=true)". Thanks for explaining.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Oct 2019 22:44:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5123/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 18:43:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

(1 comment)

> Patch Set 8: Verified-1
> 
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5114/

Some test failures need to fix: https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/8384/
Test Result (3 failures / +3)
org.apache.impala.analysis.AnalyzeKuduDDLTest.TestCreateExternalKuduTable
org.apache.impala.planner.PlannerTest.testHbase
org.apache.impala.planner.PlannerTest.testHbaseNoKeyEstimate

Hao, you can run these java tests locally according to https://cwiki.apache.org/confluence/display/IMPALA/How+to+load%2C+run%2C+and+create+new+Impala+tests
e.g. in $IMPALA_HOME:
(pushd fe && mvn test -Dtest=AnalyzeKuduDDLTest#TestCreateExternalKuduTable)
(pushd fe && mvn test -Dtest=PlannerTest#testHbase)
(pushd fe && mvn test -Dtest=PlannerTest#testHbaseNoKeyEstimate)

Looks like we broke something in HBase Tables. I'll also look into it tomorrow.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@506
PS8, Line 506: tab (x int)
Should not specify columns for creating external Kudu table.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 14:49:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

Hao Hao has uploaded a new patch set (#8) to the change originally created by Vihang Karajgaonkar. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
8 files changed, 94 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/8
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/14397/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@343
PS7, Line 343:     if (Boolean.parseBoolean(
> line too long (96 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 23:18:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 5:

> Build Failed
 > 
 > https://jenkins.impala.io/job/gerrit-code-review-checks/4797/ :
 > Initial code review checks failed. See linked job for details on
 > the failure.

The build failure seems to be due to some maven issue.
[INFO] BUILD FAILURE
[ERROR] Plugin org.apache.maven.plugins:maven-surefire-plugin:2.12.4 or one of its dependencies could not be resolved: Failed to read artifact descriptor for org.apache.maven.plugins:maven-surefire-plugin:jar:2.12.4: Could not transfer artifact org.apache.maven.plugins:maven-surefire-plugin:pom:2.12.4 from/to central (https://repo.maven.apache.org/maven2): /home/ubuntu/.m2/repository/org/apache/maven/plugins/maven-surefire-plugin/2.12.4/maven-surefire-plugin-2.12.4.pom.part (No such file or directory) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Oct 2019 19:47:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2185
PS6, Line 2185: isExternalTable
It should be updated to check for not (synchronized) tables.


http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2192
PS6, Line 2192: isExternalTable
Same here.


http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2210
PS6, Line 2210: isExternalTable
Same here.


http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
> Should this be checking for whether the table is synchronized? Are impala u
+1. I think checks in CatalogOpExecutor::createKuduTable() also need to be updated.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Oct 2019 21:13:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5114/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 04:31:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@117
PS2, Line 117: managed Kudu table
Update the comment to reflect the change and else where.


http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isExternallyManagedTable
> logically its doing the same right. It is checking if this table is not ext
Yeah, I guess I would prefer the naming 'isSynchronizedTable' as it is more clear and less misleading than 'isExternallyManagedTable'.


http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@131
PS2, Line 131: HMS
nit: missing period.


http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/Table.java@157
PS2, Line 157: external table
nit: external table.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Oct 2019 23:35:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 5:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/4797/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Oct 2019 18:11:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

(2 comments)

The test failure in AnalyzeKuduDDLTest is due to a typo.
The test failures in testHbase and testHbaseNoKeyEstimate look flaky. Cannot reproduce them in my local env and is not shown in the first GVO. I think it's not relative to this patch.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@507
PS8, Line 507: purges
typo: purge


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@508
PS8, Line 508: purges
same here: purge



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 11:10:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isExternallyManagedTable
> Would it make sense to check whether the table is managed? E.g. define this
logically its doing the same right. It is checking if this table is not externally managed which means either the table is managed or (it is external and has the external.purge.table = false/null)


http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@141
PS2, Line 141: !Boolean
             :         .parseBoolean
> Maybe I'm missing something, but if "external.table.purge" is true, shouldn
I know its a bit confusing. When external.table.purge is true, it means that Impala will treat the table (even if its external) as managed (it will drop it from Kudu, when the table is dropped). So in order to determine if this is a externally managed, we should look for external.table.purge either being unset or set to false.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Oct 2019 22:27:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4800/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Oct 2019 21:50:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

Patch Set 8:

(3 comments)

I think HDFS and HBase tables also have this issue. For example, creating a non-transactional HDFS table will finally create an external table. Then in DROP TABLE we won't drop HDFS files.

However, let's keep this fix simple and focus on kudu first.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@341
PS8, Line 341: // External table cannot have 'external.table.purge' property set, which is considered
: // equivalent to managed table.
: if (Boolean.parseBoolean(
: getTblProperties().get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE))) {
: throw new AnalysisException(String.format("Table property '%s' cannot be set to " +
: "true with an external Kudu table.", KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE));
: }
I think manually changing 'external.table.purge' should not be allowed. We should also check this in AlterTableSetTblProperties#analyze(). Please also add a test to cover it.

Fortunately, CREATE TABLE LIKE statement don't allow setting tblproperties and won't copy them. I think no other statements will modify the tblproperties.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@140
PS8, Line 140: public static boolean isSynchronizedTable(
Could you move these three functions into Table.java? I feel like it's not just a kudu issue. Tables on HDFS and HBase may also need these. However, let's keep this patch simple to just focus on Kudu. We can deal with HDFS, HBase issues in other patches.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@505
PS8, Line 505: // Cannot specify the number of replicas for external Kudu tables
Comment looks incorrect here. Maybe: External Kudu table is not allowed to set table property 'external.table.purges' to true.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 06:21:38 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 11: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 00:13:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 11: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 04:31:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4771/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Oct 2019 20:34:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4831/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 23:56:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 11:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5127/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 00:13:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5113/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 23:15:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isExternallyManagedTable
> I agree, this name was confusing to me.
Just curious to know if "synchronized table" is a valid Kudu concept? When I read it I felt its a synchronized in the sense thread-safe from concurrent updates.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 18:10:44 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4757/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Oct 2019 02:26:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@134
PS3, Line 134:  a 
nit: an


http://gerrit.cloudera.org:8080/#/c/14397/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@142
PS3, Line 142: isExternalTable(msTbl) && !Boolean
             :         .parseBoolean(msTbl.getParameters().get(TBL_PROP_EXTERNAL_TABLE_PURGE));
This should be reversed now, since a synchronized table is either managed or has the purge option set to true, right?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Oct 2019 00:56:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 10:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/4844/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 19:25:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

Hao Hao has uploaded a new patch set (#7) to the change originally created by Vihang Karajgaonkar. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
8 files changed, 93 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/7
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 10: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5123/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 23:01:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Testing:
1. Bumped up the CDP build number in a private build so that the HMS translation logic is
pulled in. Ran all the tests. Without the patch there are many Kudu tests which were
failing. After the patch none of the Kudu tests fail. There were additional Ranger tests
which failed due to the CDP bump but those were unrelated to this patch and should be
fixed as part of a separate change when the CDP build number is bumped up.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/3
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 10: Code-Review+2

> Patch Set 10:
> 
> > Patch Set 10: Verified-1
> > 
> > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5123/
> 
> The failure test TestEventProcessing.test_insert_events seems to be unrelated flaky test.

Agree with that. Codes look good to me.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Oct 2019 00:12:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

In HMS 3.1 there is a default tranformer introduced which checks the client capabilities
and transforms a table before creating it. Additionally, it also makes sure that any
managed table which is created is transactional. If a user creates a managed table which
is not transactional, it automatically converts such table as external and sets certain
table properties to mark such transformed tables.

This presents a problem for managed Kudu tables in Impala since managed and external
tables are handled differently in Kudu. Specifically, if a Kudu table is managed, certain
operations like drop table, rename table, alter table are performed on the Kudu side along
with updating the catalog. If the Kudu table is external, the Kudu operations are skipped
and only catalog side operations are performed.

When the user creates a managed Kudu table, user expects that drop table, rename table
should be updated by Impala automatically in Kudu as well. But since HMS 3 transforms such
managed tables into external, currently Impala does not perform the Kudu side operations
breaking the semantics for the user.

This patch makes changes to Catalog so that it can detect such transformed external tables
and perform Kudu side operations similar to what it was doing for managed Kudu table when
talking with previous HMS versions.

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Testing:
1. Bumped up the CDP build number in a private build so that the HMS
translation logic is pulled in. Ran all the tests. Without the patch there are many Kudu
tests which were failing. After the patch none of the Kudu tests fail. There were
additional Ranger tests which failed due to the CDP bump but those were unrelated to
this patch and should be fixed as part of a separate change when the CDP build number
is bumped up.

Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Reviewed-on: http://gerrit.cloudera.org:8080/14397
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
8 files changed, 105 insertions(+), 40 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

(6 comments)

> Patch Set 8:
> 
> (1 comment)
> 
> > Patch Set 8: Verified-1
> > 
> > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5114/
> 
> Some test failures need to fix: https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/8384/
> Test Result (3 failures / +3)
> org.apache.impala.analysis.AnalyzeKuduDDLTest.TestCreateExternalKuduTable
> org.apache.impala.planner.PlannerTest.testHbase
> org.apache.impala.planner.PlannerTest.testHbaseNoKeyEstimate
> 
> Hao, you can run these java tests locally according to https://cwiki.apache.org/confluence/display/IMPALA/How+to+load%2C+run%2C+and+create+new+Impala+tests
> e.g. in $IMPALA_HOME:
> (pushd fe && mvn test -Dtest=AnalyzeKuduDDLTest#TestCreateExternalKuduTable)
> (pushd fe && mvn test -Dtest=PlannerTest#testHbase)
> (pushd fe && mvn test -Dtest=PlannerTest#testHbaseNoKeyEstimate)
> 
> Looks like we broke something in HBase Tables. I'll also look into it tomorrow.

Thanks Quanlong. I didn't do it as I don't have a suitable environment to set up Impala (which can takes hours to do), and I think this change is obvious enough to rely on the Jenkins output.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@341
PS8, Line 341:     // External table cannot have 'external.table.purge' property set, which is considered
             :     // equivalent to managed table.
             :     if (Boolean.parseBoolean(
             :             getTblProperties().get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE))) {
             :       throw new AnalysisException(String.format("Table property '%s' cannot be set to " +
             :           "true with an external Kudu table.", KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE));
             :     }
> I think manually changing 'external.table.purge' should not be allowed. We 
Good point! Updated.


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@140
PS8, Line 140:   public static boolean isSynchronizedTable(
> Sorry, don't need to move these... Only Kudu tables are affected.
Ack


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@505
PS8, Line 505:     // Cannot specify the number of replicas for external Kudu tables
> Comment looks incorrect here. Maybe: External Kudu table is not allowed to 
Done


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@506
PS8, Line 506: tab (x int)
> Should not specify columns for creating external Kudu table.
Done


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@507
PS8, Line 507: purges
> typo: purge
Done


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@508
PS8, Line 508: purges
> same here: purge
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 18:35:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 10:

> Patch Set 10: Verified-1
> 
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5123/

The failure test TestEventProcessing.test_insert_events seems to be unrelated flaky test.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 23:08:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isExternallyManagedTable
Would it make sense to check whether the table is managed? E.g. define this as

if (KuduTable.isSynchronizedTable(table_.getMetaStoreTable()) {
  // throw error...
}

Where KuduTable.isSynchronziedTable() checks for whether it's managed _or_ whether it's external with the purge property? That's along the lines of what Kudu did in https://gerrit.cloudera.org/c/14179/8/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java#285


http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@141
PS2, Line 141: !Boolean
             :         .parseBoolean
Maybe I'm missing something, but if "external.table.purge" is true, shouldn't it be considered externally managed? Why the ! here?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Oct 2019 20:41:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5114/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 23:19:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.

Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/14397/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@120
PS2, Line 120: isExternallyManagedTable
> Just curious to know if "synchronized table" is a valid Kudu concept? When 
synchronized comes from whether a table is kept in-snyc with HMS in Kudu HMS sync feature. I don't think that terminology needs to be used here. But a terminology change would be useful to avoid confusion.

I think the confusion comes from the fact that external and managed are existing concepts on their own and "externallyManaged" uses both the terms. It would be useful to have a term which describes a table that is tightly coupled like managed tables or external purge tables are. 

I don't think it's critical to fix/change this now. With 2 +1s, this just needs and Impala committer to review and provide feedback.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 19:42:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 7: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5113/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 03:34:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

Hao Hao has uploaded a new patch set (#9) to the change originally created by Vihang Karajgaonkar. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................

IMPALA-9030: Handle translated external Kudu tables

Note that this change is in preparation of bumping up the CDP build which will be done in
a separate change. For the current CDP build number the patch is essentially a no-op.

Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
8 files changed, 105 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/14397/9
--
To view, visit http://gerrit.cloudera.org:8080/14397
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
> I agree, ideally for Kudu external+purge tables would be treated the same a
That said, if someone more familiar with Impala metadata feels that option 2 is safe, I'd prefer going down that route.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 21:23:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
Should this be checking for whether the table is synchronized? Are impala users able to create external tables today with the purge option? If so, do we need to update CatalogOpExecutor::createKuduTable() to also handle synchronized tables?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Oct 2019 21:49:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2185
PS6, Line 2185: isExternalTable
> It should be updated to check for not (synchronized) tables.
I think we should continue checking only external table here because for external kudu tables, Impala expects the table to exist in Kudu and it populates the schema from Kudu table in the newTable object.

I think the behavior if the user issues a 

create external table foo (id int) tblproperties ('external.purge.table'='true') is unclear.

if table is defined as external, impala will expect the table to exist in Kudu and hence will read the schema from that table which is what the code is doing currently and the patch doesn't really change that.


http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
> +1. I think checks in CatalogOpExecutor::createKuduTable() also need to be 
So this is interesting. Note that this method is implemented to create managed tables (it will go and create a kudu table for you) so we want to allow only the such tables to go further to line 76.

if we are checking for !KuduTable.isSynchronized(msTbl) what we are essentially checking is
that msTbl is !(managed || (external && external.purge.table=true))

which translates to !managed && !(external && external.purge.table=true)

which translates to !managed && !external || external.purge.table=false/unset

which translates to external.purge.table=false/unset

so all the statements below will be good:
create table foo (id int);
create external table foo (id int);
create external table foo (id int) tblproperties ('external.purge.table' = 'false');

We don't want this to happen since for any external table we expect the Kudu table to exist and hence the method will fail at line 88. We should really be just checking for !external tables since the code below goes ahead and creates Kudu table.

The open question here is what should happen if the user does this:

create table foo(id int) tblproperties ('external.purge.table'='true');

This in my opinion is an incorrect usage of the tbl property and we should disallow it. But Hive allows it and hence Impala has to allow it too for compatiblity.

What will happen with this SQL is it will be sent to Kudu which will create a table and HMS will eventually create a external table foo with 'external.purge.table'='true' after translation so the drop/rename will work just as same as a managed table as expected. Basically the tbl property provided by user is unnecessary and does not impact anything as expected.

I am not sure if this helps or makes it more confusing :) Let me know if see any problems with my reasoning above.

Once we agree I will add comment there to explain the same or do necessary changes.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Oct 2019 23:14:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75
PS6, Line 75: !KuduTable.isExternalTable(msTbl)
> Hmm, it will be translated to 'Preconditions.checkState(KuduTable.isSynchro
I agree, ideally for Kudu external+purge tables would be treated the same as managed table, even in creation. Otherwise, I imagine it being quite confusing how to handle the rename or deletion of an external+purge table that doesn't exist in Kudu (e.g. drop table would fail because the table doesn't exist in Kudu).

I think we should explore doing number 2, but I'm uncertain whether there are other repercussions in Impala surrounding creating an external table when a managed table was requested. As such, for now, let's just go with option 1 -- it seems easier to me, and simpler to reason about and document than the current behavior.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 21:22:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )

Change subject: IMPALA-9030: Handle translated external Kudu tables
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4790/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 21:49:01 +0000
Gerrit-HasComments: No