[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups

2019-04-18 Thread Vihang Karajgaonkar (Code Review) 
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 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Bharath Krishna 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vihang Karajgaonkar 
Gerrit-Comment-Date: Thu, 18 Apr 2019 21:48:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups

2019-04-15 Thread Bharath Krishna (Code Review) 
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 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Bharath Krishna 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vihang Karajgaonkar 
Gerrit-Comment-Date: Tue, 16 Apr 2019 04:28:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups

2019-04-15 Thread Bharath Krishna (Code Review) 
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 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Bharath Krishna 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vihang Karajgaonkar 
Gerrit-Comment-Date: Tue, 16 Apr 2019 04:26:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups

2019-04-15 Thread Vihang Karajgaonkar (Code Review) 
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 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vihang Karajgaonkar 
Gerrit-Comment-Date: Tue, 16 Apr 2019 01:48:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups

2019-04-15 Thread Impala Public Jenkins (Code Review) 
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 
Gerrit-Reviewer: Anurag Mantripragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vihang Karajgaonkar 
Gerrit-Comment-Date: Mon, 15 Apr 2019 23:55:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8419 : Fetch metastore configuration values to detect misconfigured setups

2019-04-15 Thread Bharath Krishna (Code Review) 
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