You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bharath Krishna (Code Review)" <ge...@cloudera.org> on 2019/04/15 23:13:46 UTC
[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
Bharath Krishna has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13019
Change subject: IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
......................................................................
IMPALA-8419 : Fetch metastore configuration values to
detect misconfigured setups
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
4 files changed, 135 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/4
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bh...@cloudera.com>
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 493 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/7
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 7
Gerrit-Owner: Bharath Krishna <bh...@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>
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 15:
Triggered dry-run external to verify that all the tests pass.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 15
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 07:47:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has uploaded a new patch set (#15). ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorConfigValidator.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 530 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/15
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 15
Gerrit-Owner: Bharath Krishna <bh...@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-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2918/ : 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/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 7
Gerrit-Owner: Bharath Krishna <bh...@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 21:05:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 14:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4105/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 14
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 22:39:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 15:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2998/ : 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/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 15
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 08:44:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@315
PS4, Line 315: try {
Use try-with-resources here to automatically close MetastoreClient once done using it. Is there a reason to use input parameter of MetastoreClient? Why not just use
try (MetaStoreClient msClient = catalog_.getMetaStoreClient()) {
...
} catch (TException tex) {
}
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@320
PS4, Line 320: Preconditions.checkState(configValueFromMetastore.equals(metaConf.expectedValue),
Using Preconditions and then catching IllegalStateException doesn't make a lot of sense. Perhaps just use Preconditions.
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@321
PS4, Line 321: Expected value:" +
: " %s
May be this part can also be part of the overridden toString() method
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@88
PS4, Line 88: public static final String DEFAULT_METASTORE_CONFIG_VALUE =
Why do we need this?
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@123
PS4, Line 123: public static String getMetastoreConfigValue(
May be a more useful way is to add a input argument to provide a default value which can be used by the caller
http://gerrit.cloudera.org:8080/#/c/13019/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/13019/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@253
PS4, Line 253: when(mockIMetaStoreClient.getConfigValue(config.toString(),
: MetaStoreUtil.DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(config.getExpectedValue());
Why do we care to revert the value on a mock client?
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@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: Tue, 16 Apr 2019 01:48:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorConfigValidator.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 529 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/13
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 13
Gerrit-Owner: Bharath Krishna <bh...@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-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 14:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/2982/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 14
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 23:35:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 14:
> Patch Set 14: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4105/
This code seems to have compilation error:
23:16:58 [ERROR] COMPILATION ERROR :
23:16:58 [ERROR] /home/ubuntu/Impala/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:[868,51] cannot find symbol
23:16:58 [INFO] BUILD FAILURE
23:16:58 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) on project impala-frontend: Compilation failure
23:16:58 [ERROR] /home/ubuntu/Impala/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:[868,51] cannot find symbol
23:16:58 [ERROR] symbol: method getFileName()
23:16:58 [ERROR] location: variable fd of type org.apache.impala.catalog.HdfsPartition.FileDescriptor
23:16:58 [ERROR] -> [Help 1]
23:16:58 [ERROR]
23:16:58 [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
23:16:58 [ERROR] Re-run Maven using the -X switch to enable full debug logging.
23:16:58 [ERROR]
23:16:58 [ERROR] For more information about the errors and possible solutions, please read the following articles:
23:16:58 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 14
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 00:17:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2923/ : 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/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 9
Gerrit-Owner: Bharath Krishna <bh...@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 22:17:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 14: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4105/
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 14
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 00:04:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13019/9/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/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@274
PS9, Line 274: or (TestIncorrectMetastoreEventConfigs config :
> maybe also assert the error message?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 11
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 17:39:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 11: Code-Review+1
(2 comments)
Will let Vihang take a look at it, too and I can promote it to +2.
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java
File fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java:
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@182
PS11, Line 182: class ValidationResult
Use static class
http://gerrit.cloudera.org:8080/#/c/13019/9/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/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@290
PS9, Line 290: }
: }
:
: /**
: * Test that when HiveConf.METASTORE_PARAMETER_EXCLUDE_PATTERNS contains a regex
: * that filters out any parameter required for event processing, the config
: * validation fails. Config validation should succeed when the regex does not match
: * any of the required parameters.
: */
: @Test
: public void testParameterFilterValidation() throws TException {
: MetastoreEventsProcessor mockEventsProcessor =
: Mockito.mock(MetastoreEventsProcessor.class);
:
: //Regex to filter all parameters starting with impala
: when(mockEventsProcessor.getConfigValueFromMetastore(
: METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("^impala");
: ValidationResult testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("impala*");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertTrue(testResult.isValid());
: assertFalse(testResult.getReason().isPresent());
:
: // Test with default return value
: when(mockEventsProcessor.getConfigValueFromMetastore(
: METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(DEFAULT_METASTORE_CONFIG_VALUE);
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertTrue(testResult.isValid());
: assertFalse(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("randomString1, impala"
: + ".disableHmsSync, randomString2");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: //Test when a required parameter is given as regex
: String requiredParameter = MetastoreEventPropertyKey.CATALOG_SERVICE_ID.getKey();
: when(mockEventsProcessor.getConfigValueFromMetastore(
: METASTORE_PARAMETER_EXCLUDE_PATTERNS, DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(requiredParameter);
: testResult =
> I don't feel we should add checks for all the tests. I am adding a check fo
Fair enough.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 11
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 17:48:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 507 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/9
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 9
Gerrit-Owner: Bharath Krishna <bh...@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-8419 : Fetch metastore configuration values to detect misconfigured setups
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
......................................................................
Patch Set 4:
Vihang, thanks for the review. I had a few questions on your comments so please let me know what you feel is a better choice.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 16 Apr 2019 04:28:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 9: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4072/
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 9
Gerrit-Owner: Bharath Krishna <bh...@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 04:31:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 15: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 15
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 12:39:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 16: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 16
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 19:58:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 13:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@20
PS13, Line 20: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.hasValidMetastoreConfigs;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@21
PS13, Line 21: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.verifyParametersNotFiltered;
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/13/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/13019/13/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@21
PS13, Line 21: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.DEFAULT_METASTORE_CONFIG_VALUE;
line too long (108 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@22
PS13, Line 22: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS;
line too long (114 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@23
PS13, Line 23: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.validateMetastoreConfigs;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@24
PS13, Line 24: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.validateMetastoreEventParameters;
line too long (110 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/13/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@73
PS13, Line 73: import org.apache.impala.catalog.events.EventProcessorConfigValidator.MetastoreEventConfigsToValidate;
line too long (102 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 13
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 18:14:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
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/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Reviewed-on: http://gerrit.cloudera.org:8080/13019
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorConfigValidator.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 530 insertions(+), 67 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 17
Gerrit-Owner: Bharath Krishna <bh...@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-8419 : Fetch metastore configuration values to detect misconfigured setups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2790/ : 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/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@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: Mon, 15 Apr 2019 23:55:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@222
PS4, Line 222: FIRE_EVENTS_FOR_DML
We probably also need to make sure that the parameters filter config does not filter out keys like impala.events.catalogVersion, impala.events.catalogServiceId and impala.disableHmsSync
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@315
PS4, Line 315: try {
> Do you mean to use try-with-resource in the above method which calls using
I see. May be you can then create another method called getMetastoreConfig(String key, String defaultval) here. This method can implement it using try-with-resource as I suggested above. In the test, you can then use Mockito.spy to return the dummy values when this method is called. Something like
MetastoreStoreEventsProcessor spyEventsProcessor = Mockito.spy(eventsProcessor_);
doReturn("testValue").when(spyProcessor.getMetastoreConfig());
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@123
PS4, Line 123: public static String getMetastoreConfigValue(
> I feel this is useful too when users can just compare the return value with
A more common pattern (and generic too) is to let the consumers decide what should be the default value if the configuration is not present. For example, for a boolean configuration users can choose to do getConfig(key, "false) whereas for a String configuration they can do getConfig(key, ""); If you avoid hard-coding the default value assumption it make it more usable for all the consumers.
http://gerrit.cloudera.org:8080/#/c/13019/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/13019/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@245
PS4, Line 245: toggleBooleanValueString
code assumes that config values will always be booleans. Its probably easier to just pass some dummy. Also since there are only a few configurations, it probably easier to just have multiple statements of
when(getConfig(key1)).thenReturn(val1);
when(getConfig(key1)).thenReturn(val1);
when(getConfig(key1)).thenReturn(val1);
and get rid of the loop this way there is no need to have the cleanup logic too. All this can be done by creating a simple util method which takes in validateConfig(key, mockValue, shouldSucceed). You can test both the positive and negative cases using this util method.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bh...@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: Thu, 18 Apr 2019 21:48:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/13019/7/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/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@73
PS7, Line 73: // flag to be set in the table/database parameters to disable event based metadata sync
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@20
PS7, Line 20: import static org.apache.impala.catalog.events.EventProcessorValidation.hasValidMetastoreConfigs;
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@21
PS7, Line 21: import static org.apache.impala.catalog.events.EventProcessorValidation.verifyParametersNotFiltered;
line too long (100 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/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/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@21
PS7, Line 21: import static org.apache.impala.catalog.events.EventProcessorValidation.validateMetastoreConfigs;
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@22
PS7, Line 22: import static org.apache.impala.catalog.events.EventProcessorValidation.validateMetastoreEventParameters;
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@71
PS7, Line 71: import org.apache.impala.catalog.events.EventProcessorValidation.MetastoreEventConfigsToValidate;
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1100
PS7, Line 1100: dbParams.put(MetastoreEventParameters.DISABLE_EVENT_HMS_SYNC_KEY.getKey(), dbFlag);
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1114
PS7, Line 1114: MetastoreEventParameters.DISABLE_EVENT_HMS_SYNC_KEY.getKey(), tblTransition.second);
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1621
PS7, Line 1621: alteredDb.putToParameters(MetastoreEventParameters.DISABLE_EVENT_HMS_SYNC_KEY.getKey(),
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 7
Gerrit-Owner: Bharath Krishna <bh...@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:22:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 9:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@20
PS9, Line 20: import static org.apache.impala.catalog.events.EventProcessorValidator.hasValidMetastoreConfigs;
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@21
PS9, Line 21: import static org.apache.impala.catalog.events.EventProcessorValidator.verifyParametersNotFiltered;
line too long (99 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/9/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/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@21
PS9, Line 21: import static org.apache.impala.catalog.events.EventProcessorValidator.DEFAULT_METASTORE_CONFIG_VALUE;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@22
PS9, Line 22: import static org.apache.impala.catalog.events.EventProcessorValidator.validateMetastoreConfigs;
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@23
PS9, Line 23: import static org.apache.impala.catalog.events.EventProcessorValidator.validateMetastoreEventParameters;
line too long (104 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@72
PS9, Line 72: import org.apache.impala.catalog.events.EventProcessorValidator.MetastoreEventConfigsToValidate;
line too long (96 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 9
Gerrit-Owner: Bharath Krishna <bh...@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 21:53:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2972/ : 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/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 13
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 18:58:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2968/ : 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/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 11
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 18:31:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 16:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4117/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 16
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 14:28:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 16
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 14:28:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 520 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/11
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 11
Gerrit-Owner: Bharath Krishna <bh...@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-8419 : Validate event processing related configurations
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 13: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 13
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 18:49:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
IMPALA-8419 : Validate event processing related configurations
Using the Metastore API to get the configuration values, verify that the
configurations needed for event processing are set correctly. Also check
that the parameters required for event processing is not filtered out by
the Hive config METASTORE_PARAMETER_EXCLUDE_PATTERNS.
This validation is done while creating the event processor and throws
CatalogException if the configuration is incorrect.
Testing
- Added unit tests
Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
---
A fe/src/main/java/org/apache/impala/catalog/events/EventProcessorConfigValidator.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
7 files changed, 530 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/13019/14
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 14
Gerrit-Owner: Bharath Krishna <bh...@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-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@20
PS11, Line 20: import static org.apache.impala.catalog.events.EventProcessorValidator.hasValidMetastoreConfigs;
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@21
PS11, Line 21: import static org.apache.impala.catalog.events.EventProcessorValidator.verifyParametersNotFiltered;
line too long (99 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/11/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/13019/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@21
PS11, Line 21: import static org.apache.impala.catalog.events.EventProcessorValidator.DEFAULT_METASTORE_CONFIG_VALUE;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@22
PS11, Line 22: import static org.apache.impala.catalog.events.EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS;
line too long (108 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@23
PS11, Line 23: import static org.apache.impala.catalog.events.EventProcessorValidator.validateMetastoreConfigs;
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@24
PS11, Line 24: import static org.apache.impala.catalog.events.EventProcessorValidator.validateMetastoreEventParameters;
line too long (104 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/11/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@73
PS11, Line 73: import org.apache.impala.catalog.events.EventProcessorValidator.MetastoreEventConfigsToValidate;
line too long (96 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 11
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 17:34:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 14:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@20
PS14, Line 20: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.hasValidMetastoreConfigs;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@21
PS14, Line 21: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.verifyParametersNotFiltered;
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/14/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/13019/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@21
PS14, Line 21: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.DEFAULT_METASTORE_CONFIG_VALUE;
line too long (108 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@22
PS14, Line 22: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS;
line too long (114 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@23
PS14, Line 23: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.validateMetastoreConfigs;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@24
PS14, Line 24: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.validateMetastoreEventParameters;
line too long (110 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/14/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@81
PS14, Line 81: import org.apache.impala.catalog.events.EventProcessorConfigValidator.MetastoreEventConfigsToValidate;
line too long (102 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 14
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 22:40:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 9:
(25 comments)
In general CR looks good, feedback is mostly about nits and Impala code style conformance.
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java
File fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java:
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@51
PS9, Line 51: private String conf, expectedValue;
make these final and use _ suffix since that's Impala convention for member variables.
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@62
PS9, Line 62: public String getExpectedValue() {
Put this in one line, see the comment below.
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@69
PS9, Line 69: public String getConf() {
: return conf;
: }
Impala code style to put this into one line when possible:
public String getConf() { return conf_; }
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@79
PS9, Line 79: // Hive config name that can have regular expressions to filter out parameters.
: String METASTORE_PARAMETER_EXCLUDE_PATTERNS =
: "hive.metastore.notification.parameters.exclude.patterns";
:
: // Default config value (currently just an empty String) to be returned from Metastore
: // when the given config is not set.
: String DEFAULT_METASTORE_CONFIG_VALUE = "";
should these be static final?
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@112
PS9, Line 112: static ValidationResult validateMetastoreEventParameters(
: MetastoreEventsProcessor eventsProcessor) {
add @VisibleForTesting since it looks like it's not private because you want to test it?
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@182
PS9, Line 182: class ValidationResult
the general recommendation is to put this as an inner class to EventProcessorValidator
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@184
PS9, Line 184: // Boolean indicating whether the validation was success or failure.
: private boolean valid;
: // Reason indicating the failure of validation. Only relevant when the validation fails.
: private String reason;
use _ suffix naming convention
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@190
PS9, Line 190: this.valid = valid;
add Preconditions.checkNotNull(valid) since "valid" can't be null
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@195
PS9, Line 195: this.valid = true;
nit: ValidationResult(true, null) is probably better
http://gerrit.cloudera.org:8080/#/c/13019/9/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/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@77
PS9, Line 77:
: private String key;
same comment about suffix naming convention
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@80
PS9, Line 80: public String getKey() {
: return key;
: }
nit: usually method comes before the constructor
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@431
PS9, Line 431: * returns
: * the database property
nit: we can join these two lines
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@251
PS9, Line 251: result.getReason().isPresent() ?
: result.getReason().get()
: : "Event Processor initialization failed during validation check."
nit: result.getReason.orElse() is shorter
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@304
PS9, Line 304: @VisibleForTesting
nit: add one extra space
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@617
PS9, Line 617:
nit: remove 4 spaces
http://gerrit.cloudera.org:8080/#/c/13019/9/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/13019/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3094
PS9, Line 3094: .getKey()
nit: this can be moved to L3093
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3109
PS9, Line 3109: .getKey()));
nit: this can be moved to L3108
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@117
PS9, Line 117: public static String getMetastoreConfigValue(
: IMetaStoreClient client, String config, String defaultVal)
: throws TException {
: return client.getConfigValue(config, defaultVal);
: }
fix indentation
http://gerrit.cloudera.org:8080/#/c/13019/9/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/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@159
PS9, Line 159: private String conf, correctValue, incorrectValue;
same comment about suffix naming convention
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@274
PS9, Line 274: assertTrue(testResult.getReason().isPresent());
maybe also assert the error message?
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@276
PS9, Line 276: when(mockEventsProcessor.getConfigValueFromMetastore(config.conf,
: DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(config.correctValue);
not quite sure the point of this
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@290
PS9, Line 290: MetastoreEventsProcessor mockEventsProcessor =
: Mockito.mock(MetastoreEventsProcessor.class);
:
: //Regex to filter all parameters starting with impala
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("^impala");
: ValidationResult testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("impala*");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertTrue(testResult.isValid());
: assertFalse(testResult.getReason().isPresent());
:
: // Test with default return value
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(DEFAULT_METASTORE_CONFIG_VALUE);
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertTrue(testResult.isValid());
: assertFalse(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("randomString1, impala"
: + ".disableHmsSync, randomString2");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn("impala.events.catalogServiceId");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
:
: when(mockEventsProcessor.getConfigValueFromMetastore(
: EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS,
: DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("^impala.events"
: + ".catalogServiceId");
: testResult =
: validateMetastoreEventParameters(mockEventsProcessor);
: assertFalse(testResult.isValid());
: assertTrue(testResult.getReason().isPresent());
assert the error messages as well?
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@354
PS9, Line 354:
nit: remove extra empty new line
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@777
PS9, Line 777:
nit: remove 4 spaces
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java
File fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java:
http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java@30
PS9, Line 30:
nit: too overly indented, remove 4 spaces
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 9
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 16:03:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 13: Code-Review+1
Thanks for all the fixes. LGTM. I will give a +2 and merge only after the dependent patch is merged and rebased against the dependent patch.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 13
Gerrit-Owner: Bharath Krishna <bh...@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: Mon, 29 Apr 2019 18:51:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
Posted by "Bharath Krishna (Code Review)" <ge...@cloudera.org>.
Bharath Krishna has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@315
PS4, Line 315: try {
> Use try-with-resources here to automatically close MetastoreClient once don
Do you mean to use try-with-resource in the above method which calls using MetastoreClient?
I added the client as an input param so that I can Mock the test and extract out this method as independent.
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@88
PS4, Line 88: public static final String DEFAULT_METASTORE_CONFIG_VALUE =
> Why do we need this?
Will remove this if the below comment of mine doesn't sound like a good approach
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@123
PS4, Line 123: public static String getMetastoreConfigValue(
> May be a more useful way is to add a input argument to provide a default va
I feel this is useful too when users can just compare the return value with MetastoreUtil.DEFAULT_METASTORE_CONFIG_VALUE
Do you feel it would be better to add two methods :
getMetastoreConfigValue(client, config) (which calls the below method with DEFAULT_METASTORE_CONFIG_VALUE)
and
getMetastoreConfigValue(client, config, defaultVal)
Or should I just have the latter one?
http://gerrit.cloudera.org:8080/#/c/13019/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/13019/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@128
PS1, Line 128: import org.slf4j.LoggerFactory;
Use only the required imports
http://gerrit.cloudera.org:8080/#/c/13019/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/13019/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@253
PS4, Line 253: when(mockIMetaStoreClient.getConfigValue(config.toString(),
: MetaStoreUtil.DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(config.getExpectedValue());
> Why do we care to revert the value on a mock client?
Here, I am iterating over each Config, and just toggling its value. If I don't revert the value back, in the next iteration it will throw an exception in the previous value itself. Please let me know if it makes sense to do that way or if should consider some other alternative.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 16 Apr 2019 04:26:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4072/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 9
Gerrit-Owner: Bharath Krishna <bh...@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 23:47:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 7:
(12 comments)
Overall looks good. Few comments below and then good to go from my side.
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java
File fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java:
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@42
PS7, Line 42: EventProcessorValidation
a better name could be EventProcessorConfigValidator
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@42
PS7, Line 42: interface
general convention in Impala is not to use package-private classes (although I think its a good idea to have them) For consistency, please change these to public. Same with the members of this class. Try to keep the members private unless its not possible.
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@59
PS7, Line 59: getExpectedValue
method doc
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@74
PS7, Line 74: String
private static final
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@115
PS7, Line 115: MetastoreEventParameters
nit, formatting is off
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@120
PS7, Line 120: filtered
s/filtered/filtered out/
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@157
PS7, Line 157: Found
would be good to provided the expected value here as well
http://gerrit.cloudera.org:8080/#/c/13019/7/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/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@68
PS7, Line 68: MetastoreEventParameters
using parameters and properties interchangebly is confusing. may be rename this to MetastoreEventPropertyKey
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@70
PS7, Line 70: CATALOG_VERSION_PROP_KEY
_PROP_KEY is redundant if you name the enum as MetastoreEventPropertyKey. SAme for others.
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@253
PS7, Line 253: Runtime error..
redundant to have this text here. can be removed.
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:
http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@88
PS7, Line 88: DEFAULT_METASTORE_CONFIG_VALUE
I am not convinced of the value of having this. Why can't we just use empty string which could be private to the config validator?
http://gerrit.cloudera.org:8080/#/c/13019/7/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/13019/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@336
PS7, Line 336: impala
add a test case for ^impala as well since it matches the disableHmsSync paramter
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 7
Gerrit-Owner: Bharath Krishna <bh...@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:23:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 15:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@20
PS15, Line 20: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.hasValidMetastoreConfigs;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@21
PS15, Line 21: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.verifyParametersNotFiltered;
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/15/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/13019/15/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@21
PS15, Line 21: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.DEFAULT_METASTORE_CONFIG_VALUE;
line too long (108 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@22
PS15, Line 22: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS;
line too long (114 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@23
PS15, Line 23: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.validateMetastoreConfigs;
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@24
PS15, Line 24: import static org.apache.impala.catalog.events.EventProcessorConfigValidator.validateMetastoreEventParameters;
line too long (110 > 90)
http://gerrit.cloudera.org:8080/#/c/13019/15/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@81
PS15, Line 81: import org.apache.impala.catalog.events.EventProcessorConfigValidator.MetastoreEventConfigsToValidate;
line too long (102 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 15
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 07:47:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8419 : Validate event processing related configurations
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 15
Gerrit-Owner: Bharath Krishna <bh...@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: Tue, 30 Apr 2019 14:28:06 +0000
Gerrit-HasComments: No