You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sharanitha Harish (Code Review)" <ge...@cloudera.org> on 2019/07/30 01:30:00 UTC
[Impala-ASF-CR] IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
Sharanitha Harish has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13952
Change subject: IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
......................................................................
IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
This patch aims to improve the validation of configuration keys from the metastore server.
The issue with configuration validation in IMPALA-8559 is that it validates one configuration
at a time and fails as soon as there is a validation error. Since there are more than one
configuration keys to validate, user may have to restart HMS again and again if there are
multiple configuration changes which are needed. This is not a great user experience.
This patch presents all of the incorrect configuration validations and results together
in case of failures so that user can change all the required changes in one go.
Added a test testValidateConfigs() to assert if multiple incorrect values are thrown
together according the MetastoreShim.getMajorVersion().
Change-Id: I52d07bce88f9332a34bfe2f9b31570203485d544
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
2 files changed, 28 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/13952/1
--
To view, visit http://gerrit.cloudera.org:8080/13952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I52d07bce88f9332a34bfe2f9b31570203485d544
Gerrit-Change-Number: 13952
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13952 )
Change subject: IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13952/1/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/13952/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@281
PS1, Line 281: results.size(
Instead of printing the ArrayList directly, can you add a log at error level here directly LOG.error("Found {} metastore configuration(s) incorrectly, results.size()) and then the loop below just do a LOG.error(invalidConfig.getReason());
If you do that way, you can also get rid of List<String> errorMessages.
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@286
PS1, Line 286: throw new CatalogException(String.format("Found %d metastore configuration(s) incorrectly"+"" +
> line too long (101 > 90)
Please fix this line width issue. Use the IDE to reformat the changed code block.
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@287
PS1, Line 287: " set. Event processing cannot be started. See error log for more details.", results.size()));
> line too long (108 > 90)
Please fix this line width issue. Use the IDE to reformat the changed code block.
--
To view, visit http://gerrit.cloudera.org:8080/13952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52d07bce88f9332a34bfe2f9b31570203485d544
Gerrit-Change-Number: 13952
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 19:16:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13952 )
Change subject: IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4070/ : 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/13952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52d07bce88f9332a34bfe2f9b31570203485d544
Gerrit-Change-Number: 13952
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 02:41:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13952 )
Change subject: IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13952/1/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/13952/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@286
PS1, Line 286: throw new CatalogException(String.format("Found %d metastore configuration(s) incorrectly"+"" +
line too long (101 > 90)
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@287
PS1, Line 287: " set. Event processing cannot be started. See error log for more details.", results.size()));
line too long (108 > 90)
http://gerrit.cloudera.org:8080/#/c/13952/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/13952/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@221
PS1, Line 221: * Testing if validateConfigs() is working as expected. If multiple configuration keys of the metastore are incorrect,
line too long (120 > 90)
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@222
PS1, Line 222: * then we collect all the configuration validations together and then present the results together in case of
line too long (112 > 90)
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@223
PS1, Line 223: * failures so that user can change all the required changes in one go.This test would assert both for
line too long (104 > 90)
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@231
PS1, Line 231: Mockito.when(mockMetastoreEventsProcessor.getConfigValueFromMetastore(configKey, "")).thenReturn("false");
line too long (112 > 90)
http://gerrit.cloudera.org:8080/#/c/13952/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@238
PS1, Line 238: assertTrue(e.getMessage().contains(String.format(errorMessage, majorVersion >= 2 ? 1 : 2)));
line too long (100 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52d07bce88f9332a34bfe2f9b31570203485d544
Gerrit-Change-Number: 13952
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 01:30:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has abandoned this change. ( http://gerrit.cloudera.org:8080/13952 )
Change subject: IMPALA-8761 Configuration validation introduced in IMPALA-8559 can be improved
......................................................................
Abandoned
IMPALA-8761 has been resolved by https://gerrit.cloudera.org/#/c/14240/
--
To view, visit http://gerrit.cloudera.org:8080/13952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I52d07bce88f9332a34bfe2f9b31570203485d544
Gerrit-Change-Number: 13952
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>