You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2019/04/17 00:22:59 UTC
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
xiaomeng@cloudera.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13049
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
IMPALA-8149 : Add support for alter_database events
This change add support for alter_database events in two parts:
One is adding catalogServiceId and catalogVersion in db parameters when alter database.
The other is adding alter database event, check if it's self event during process, if true do nothing, if false replace caralog cached db with event db.
Testing:
Enabled testAlterDisableFlagFromDb in MetastoreEventsProcessorTest.
Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 254 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/13049/1
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6:
I think this was probably IMPALA-8466
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 16:49:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 00:30:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
xiaomeng@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 5:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@757
PS4, Line 757: * If tblName is null, removes version number from database.
> We can say:
Done
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@445
PS1, Line 445: otected static Stri
> We can just leave it for now. Removing the check is wrong if params can be
Done
http://gerrit.cloudera.org:8080/#/c/13049/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/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3805
PS1, Line 3805: if (db == null) {
> I think removing is fine. Probably we can change the Preconditions.checkSta
Done
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3812
PS1, Line 3812: default:
> Moved it under set_owner which lock db first.
Done
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3813
PS1, Line 3813: throw new UnsupportedOperationException(
> I copied the format in alterTable method which update catalogServiceIdentif
Done
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@343
PS4, Line 343: est
> nit: fix indentation
I was using clang tool do formatting, I guess it takes each argument as indent 4, the inside argument break indented 4 more spaces on original.
From Google: "two continuation lines use the same indentation level if and only if they begin with syntactically parallel elements."
So I think it make sense to indent this way.
What do you think?
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@345
PS4, Line 345: eventsProcessor_.processEvents();
> I feel we should add an additional test, here we are testing if the Notific
Done
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1481
PS4, Line 1481: }
> Can we check if possible that the counter MetastoreEventsProcessor.EVENTS_S
Done
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 22:47:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 22:32:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2890/ : 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/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 19:10:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/Db.java@524
PS1, Line 524: + "could cause unnecessary database invalidation when the event is processed",
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 00:23:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 5: Code-Review+1
Thanks Xiaomeng for the updates in patch. LGTM.
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 00:03:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@767
PS1, Line 767: * @param tblName table name
We need to update the comment to explain that it can remove version for both DB and Table. Also, I see that if tblName is null, it will remove version from DB, which needs to be added in javadoc too.
http://gerrit.cloudera.org:8080/#/c/13049/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/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3805
PS1, Line 3805: Preconditions.checkState(dbName != null && !dbName.isEmpty(),
I think this check is redundant, as it is done inside getDb() call below?
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1448
PS1, Line 1448: @Test
we can add more tests which do alter database from Impala, and verify that it works as expected.
What about the disabled test testAlterDatabaseEvents ? Can it be enabled.
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Apr 2019 01:02:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@978
PS1, Line 978: public void process() throws DatabaseNotFoundException, CatalogException {
Looks like DatabaseNotFoundException is already captured in CatalogException, so probaly we can remove DatabaseNotFoundException from throws.
http://gerrit.cloudera.org:8080/#/c/13049/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/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3812
PS1, Line 3812: long newCatalogVersion = catalog_.incrementAndGetCatalogVersion();
Just asking to understand this part:
Do we need to lock the DB when we update the version?
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3813
PS1, Line 3813: addCatalogServiceIdentifiers(db, catalog_.getCatalogServiceId(), newCatalogVersion);
I see that the only alter operation here is SET_OWNER, so shouldn't we update the catalogServiceIdentifiers only when the operation is ALTER DB SET OWNER? Otherwise, why should we just update the version?
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3844
PS1, Line 3844: synchronized (metastoreDdlLock_) {
Should we do addCatalogServiceIdentifiers after acquiring the metastoreDdlLock_ ?
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Apr 2019 03:29:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@755
PS3, Line 755: * Removes a given version number from the catalog database/table's list of versions
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@756
PS3, Line 756: * for in-flight events.
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/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/13049/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@369
PS3, Line 369: case COMMENT_ON:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@370
PS3, Line 370: alterCommentOn(ddlRequest.getComment_on_params(), response);
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@370
PS3, Line 370: alterCommentOn(ddlRequest.getComment_on_params(), response);
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@371
PS3, Line 371: break;
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3800
PS3, Line 3800: private void alterDatabase(TAlterDbParams params, TDdlExecResponse response)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/13049/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3807
PS3, Line 3807: }
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 18:28:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
xiaomeng@cloudera.com has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
IMPALA-8149 : Add support for alter_database events
This change adds support for alter_database events in two parts:
One is adding catalogServiceId and catalogVersion in db parameters when
alter database.
The other is adding alter database event, check if it's self event
during process, if true do nothing, if false replace caralog cached db
with event db.
Testing:
Enabled testAlterDisableFlagFromDb in MetastoreEventsProcessorTest.
Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 265 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/13049/3
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2945/ : 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/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 23:38:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
I will give a chance for Bharath K take a look at it, too. I can promote to a +2 afterwards.
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@343
PS4, Line 343: est
> I was using clang tool do formatting, I guess it takes each argument as ind
I don't want to be a stickler. I'm cool with this.
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 23:59:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
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/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
IMPALA-8149 : Add support for alter_database events
This change adds support for alter_database events in two parts:
One is adding catalogServiceId and catalogVersion in db parameters when
alter database.
The other is adding alter database event, check if it's self event
during process, if true do nothing, if false replace caralog cached db
with event db.
Testing:
Enabled testAlterDisableFlagFromDb in MetastoreEventsProcessorTest.
Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Reviewed-on: http://gerrit.cloudera.org:8080/13049
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
6 files changed, 337 insertions(+), 111 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 7
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4096/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 16:49:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 00:30:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2903/ : 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/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 04:37:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4092/
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 05:28:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 1:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13049/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13049/1//COMMIT_MSG@9
PS1, Line 9: add
typo: adds
http://gerrit.cloudera.org:8080/#/c/13049/1//COMMIT_MSG@8
PS1, Line 8:
: This change add support for alter_database events in two parts:
: One is adding catalogServiceId and catalogVersion in db parameters when alter database.
: The other is adding alter database event, check if it's self event during process, if true do nothing, if false replace caralog cached db with event db.
nit: try to keep it within 72 characters
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@767
PS1, Line 767: * @param tblName table name
> We need to update the comment to explain that it can remove version for bot
+1. I'm also not quite clear why we need to remove the version for in-flight events when tblName == null. Same as above for getInFlightVersionsForEvents.
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/Db.java@521
PS1, Line 521: ==
nit: >= is probably better
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@263
PS1, Line 263: protected List<Long> pendingVersionNumbersFromCatalog_ = Collections.EMPTY_LIST;
make it final?
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@272
PS1, Line 272: event.getTableName()
can event.getTableName() be null? If not, we should do Preconditions.checkNotNull(event.getTableName()) similar to L271
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@405
PS1, Line 405: if (versionNumberFromEvent_ == -1 || pendingVersionNumbersFromCatalog_.isEmpty())
: return false;
nit: if it spans across multiple lines, use {}
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@408
PS1, Line 408: if (catalog_.getCatalogServiceId().equals(serviceIdFromEvent_)) {
: // service id is a match. Now check if the event version is what we expect
: // in the list
: if (pendingVersionNumbersFromCatalog_.get(0).equals(versionNumberFromEvent_)) {
can be combined with && to reduce the nestedness
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@439
PS1, Line 439: protected void initSelfEventIdentifiersFromEvent() {
: throw new UnsupportedOperationException("Please override this method in subclass");
: }
making it an abstract is better since we get compile-time vs runtime error.
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@445
PS1, Line 445: if (params == null)
This code is a bit weird that "params" can be null. Usually it should be an empty map instead.
http://gerrit.cloudera.org:8080/#/c/13049/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/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@370
PS1, Line 370: break;
nit: put it in the new line
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3805
PS1, Line 3805: Preconditions.checkState(dbName != null && !dbName.isEmpty(),
> I think this check is redundant, as it is done inside getDb() call below?
checkState is also not quite correct. It's more like checkArgument.
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Apr 2019 18:55:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 4:
(4 comments)
Overall looks good, leaving a few comments..
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@757
PS4, Line 757: * If tblName is null, applies to database.
We can say:
if tblName is null, removes version number from database.
if tblName is not null and Table is not incomplete, removes version number from Table.
http://gerrit.cloudera.org:8080/#/c/13049/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/13049/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3805
PS1, Line 3805: if (db == null) {
> Remove it should be fine?
I think removing is fine. Probably we can change the Preconditions.checkState() in catalog_.getDb to PreConditions.checkArguments() as Fredy suggested?
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@345
PS4, Line 345:
I feel we should add an additional test, here we are testing if the Notification Processor is successfully processing the notification events generated from Hive.
We are not triggering the AlterDatabase operation from Impala in this test. (We need to do something similar to MetastoreEventsProcessorTest#alterTableAddColsFromImpala).
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1481
PS4, Line 1481: eventsProcessor_.processEvents();
Can we check if possible that the counter MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC is updated correctly here, as we are skipping some events?
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 20:46:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4092/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 00:30:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/Db.java@523
PS4, Line 523: + " its max capacity %d. Ignoring add request for version number %d. This "
: + "could cause unnecessary database invalidation when the event is "
:
nit: fix indentation, 4 spaces for continued indentation
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@439
PS1, Line 439: protected void initSelfEventIdentifiersFromEvent() {
: throw new UnsupportedOperationException("Please override this method in subclass");
: }
> I tried to make it abstract, but some subclasses don't need call initSelfEv
In general, it's always a good idea to force the implementers to make a conscious decision whether or not to override for compile-time safety rather than forgetting to override the method and get a runtime exception. However, I have no string opinion on this since the subclasses are isolated. Perhaps, we can rename the exception message to use:
throw new UnsupportedOperationException(String.format(
"%s is not supported", ClassUtil.getMethodName()));
http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@445
PS1, Line 445: return params.getOr
> So shall I remove this check? As empty case is handled by next line getOrDe
We can just leave it for now. Removing the check is wrong if params can be null and L446 will throw an NPE.
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@343
PS4, Line 343:
nit: fix indentation
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 17:36:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
xiaomeng@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 4:
Thanks a lot for reviewing Fredy and Bharath!
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 00:55:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 6:
> Patch Set 6: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4092/
Failure in unrelated test, possibly flaky test: ImpalaBeeswaxException: ImpalaBeeswaxException: INNER EXCEPTION: <class 'beeswaxd.ttypes.BeeswaxException'> MESSAGE: TableLoadingException: Loading file and block metadata for 1 paths for table cachedb.cached_tbl_part: failed to load 1 paths. Check the catalog server log for more details.
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 13:32:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2809/ : 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/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 01:06:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
xiaomeng@cloudera.com has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
IMPALA-8149 : Add support for alter_database events
This change adds support for alter_database events in two parts:
One is adding catalogServiceId and catalogVersion in db parameters when
alter database.
The other is adding alter database event, check if it's self event
during process, if true do nothing, if false replace caralog cached db
with event db.
Testing:
Enabled testAlterDisableFlagFromDb in MetastoreEventsProcessorTest.
Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
6 files changed, 337 insertions(+), 111 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/13049/5
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-8149 : Add support for alter database events
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
xiaomeng@cloudera.com has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events
......................................................................
IMPALA-8149 : Add support for alter_database events
This change adds support for alter_database events in two parts:
One is adding catalogServiceId and catalogVersion in db parameters when
alter database.
The other is adding alter database event, check if it's self event
during process, if true do nothing, if false replace caralog cached db
with event db.
Testing:
Enabled testAlterDisableFlagFromDb in MetastoreEventsProcessorTest.
Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 261 insertions(+), 108 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/13049/4
--
To view, visit http://gerrit.cloudera.org:8080/13049
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b
Gerrit-Change-Number: 13049
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <xi...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>