You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/06/01 18:04:27 UTC
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated June 1, 2017, 6:04 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed review comments and also refactored the code more.
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
Diff: https://reviews.apache.org/r/59508/diff/3/
Changes: https://reviews.apache.org/r/59508/diff/2-3/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
> On June 4, 2017, 10:03 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 128 (patched)
> > <https://reviews.apache.org/r/59508/diff/3/?file=1738638#file1738638line128>
> >
> > This looks suspicious - why we are not using exceptions here to report errors?
I think Kalyan's reason is: 1) all notification from meta store are valid. If Sentry cannot handle it, throwing exception will cause any difference to meta store. We will swallow it eventually. 2) Throwing exception may stop processing next notification event.
The alternative to Kalyan's approach is to throw exception at each event processing. And catch it in the loop of processing the events, log such exception, and move on to next event processing. So we have more info on what's going on.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176885
-----------------------------------------------------------
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176885
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 128 (patched)
<https://reviews.apache.org/r/59508/#comment250413>
This looks suspicious - why we are not using exceptions here to report errors?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 49 (patched)
<https://reviews.apache.org/r/59508/#comment250391>
This class is not documented - please explain what is this class doing.
Also formatting - the class doesn't start on the first column
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 55 (patched)
<https://reviews.apache.org/r/59508/#comment250392>
It is usually better to separte object construction from connection establishement.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 78 (patched)
<https://reviews.apache.org/r/59508/#comment250399>
Can we handle simple (non-kerberos) case first and then deal with more complicated kerberos case without if clause?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 144 (patched)
<https://reviews.apache.org/r/59508/#comment250400>
Note that this may throw exception preventing kerberos context from being closed.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 167 (patched)
<https://reviews.apache.org/r/59508/#comment250401>
The name suggests that there is a problem here. Why combine two separate operations into one?
I think there should be
fetchSnapshot() and storeSnapshot() - the first one belongs here, the second one doesn't.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 208 (patched)
<https://reviews.apache.org/r/59508/#comment250402>
We only need to store full snapshot if the event ID didn't change in the process
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 235 (patched)
<https://reviews.apache.org/r/59508/#comment250404>
What are "new notifications"
This probably should be
"Get all HMS notifications since specified one"
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 241 (patched)
<https://reviews.apache.org/r/59508/#comment250403>
All methods here should probably be package-private rather then public
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 247 (patched)
<https://reviews.apache.org/r/59508/#comment250405>
reverse the condition and return immediately if it isn't.
if (eventId.getEventId() <= lastProcessedNotificationID) {
return Collections.emptyList();
}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 251 (patched)
<https://reviews.apache.org/r/59508/#comment250406>
Combine the two conditions into one if statement
if (response.isSetEvents() &&
(!response.getEvents().isEmpty() && (lastProcessedNotificationID != lastLoggedEventId))) {
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 72 (original), 41 (patched)
<https://reviews.apache.org/r/59508/#comment250395>
Do we still need this?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 75 (original), 44 (patched)
<https://reviews.apache.org/r/59508/#comment250394>
This comment is now detached from the variable
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 111 (original), 77 (patched)
<https://reviews.apache.org/r/59508/#comment250396>
Should check for (client != null)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 256 (original), 123 (patched)
<https://reviews.apache.org/r/59508/#comment250397>
Combine declaration with assignment
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 257 (original), 124 (patched)
<https://reviews.apache.org/r/59508/#comment250398>
What does it mean - create a full snapshot if there is one?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 78 (patched)
<https://reviews.apache.org/r/59508/#comment250414>
Why not just this.conf?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 111 (patched)
<https://reviews.apache.org/r/59508/#comment250415>
Combine declaraton with assignment
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 126 (patched)
<https://reviews.apache.org/r/59508/#comment250416>
These guys can be cached at constructor
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 225 (patched)
<https://reviews.apache.org/r/59508/#comment250410>
What does it mean "constructs name"?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 227 (patched)
<https://reviews.apache.org/r/59508/#comment250411>
It looks like the callers do not bother tocheck if it is null. Should it return an empty string itself? Or throw exception?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 230 (patched)
<https://reviews.apache.org/r/59508/#comment250407>
try pronouncing authzble over the phone :)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 231 (patched)
<https://reviews.apache.org/r/59508/#comment250412>
How about this:
public static String getAuthzObj(TSentryAuthorizable authorizable) {
String dbName = authorizable.getDb();
if (SentryStore.isNULL(dbName)) {
return null;
}
String tblName = authorizable.getTable();
return SentryStore.isNULL(tblName) ?
dbName.toLowerCase() :
(dbName + "." + authorizable.getTable()).toLowerCase();
}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 232 (patched)
<https://reviews.apache.org/r/59508/#comment250408>
return null immediately if getDb is null
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Line 222 (original), 243 (patched)
<https://reviews.apache.org/r/59508/#comment250409>
Missing newline
- Alexander Kolbasov
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
> On June 20, 2017, 11:12 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 206 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line206>
> >
> > If the fetchFullUpdate() snapshot ignores the failure, why we have to throw an exception here?
> >
> > This might confuse users when there are empty snapshots, right? Isn't better just to ignore the error and return an empty map list as well?
>
> kalyan kumar kalvagadda wrote:
> if the shapshot has no entries, HMSFOllower should not proceed futhur and just return.
Understand, but having an empty snapshot on HMS does not mean an error happened. Throwing exceptions should be done on special cases when the caller must be notified about an error during the code execution. Also, throwing exceptions is expensive, so having a function to throw an error just to notify the caller there was nothing to do then it looks like a bad design. The HMSFollower can handle empty snapshots, so why not returning an empty snapshot?
> On June 20, 2017, 11:12 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 648 (original), 270 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754607#file1754607line698>
> >
> > Couldn't be this a IOException? It is attempting to persist (write/output) to a DB, so it looks like IOException make sense.
>
> kalyan kumar kalvagadda wrote:
> failing to write to sentry could fail for a lot reason, throwing I/O exception doesn't seem to be correct.
Yes, but the HMSFollowerPersistenceException and IOException are not different. Both are saying a writing failure happend and has a message error. I am trying to avoid a new exception class if it is not needed by anyone else. Custom exceptions are useful to notify the caller what really happened, and they can have extra information about the error, but in this refactor class, i don't see what the HMSFollower can do different with a custom exception.
See https://blogs.msdn.microsoft.com/jaredpar/2008/10/20/custom-exceptions-when-should-you-create-them/
> On June 20, 2017, 11:12 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line96>
> >
> > Remove space before the parethesis. "setAuthzConf (Configuration conf)"
> >
> > Btw, if this method exists now, should we call it from the constructor? Seems we are duplicating code here.
>
> kalyan kumar kalvagadda wrote:
> I have added it for testing purposes only. If it is re-used in constrctor, it can not be documented as test only function.
> it can be wrongly used later.
It doesn't make sense. @VisibleForTesting can be still used if you call this function from the constructor. Are ther compilation errors if we do?
Also, I don't understand the method. It gets a boolean value from the authzConf object, and IF it is true, then it returns true? I think Boolean.parseBoolean() does that for you, doesn't it? this function is ideanticall from what part of the constructor does. I still think that even if we use code only for testing, if it is part of the private methods, then it should be used.
> On June 20, 2017, 11:12 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Lines 148 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line150>
> >
> > Shouldn't this be a warning instead of error? Error implies that something bad happened on the code. Here, we're just ignoring these invalid requests. A warning is more appropiate.
> >
> > The same for the rest of the notifications requests.
>
> kalyan kumar kalvagadda wrote:
> Sentry code doesn't use warn log level much. I was not sure when would it appropariate to log it as a warning.
What do you mean Sentry doesn't use warn logs? This is part of the log4j interface, isn't it?
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178434
-----------------------------------------------------------
On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/5/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178434
-----------------------------------------------------------
Thanks Kalyan, this is an interesting refactoring, thanks for the hard work your putting here. Btw, I have the feeling that this refactor can be splitted into 3 different patches; the HMSClientWrapper, the NotificationProcessor and the HMSFollower itself. Shouldn't it be better to split this to allow better code reviews? It's just a recommendation and is optional.
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 45 (patched)
<https://reviews.apache.org/r/59508/#comment252393>
Unused import.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java
Line 24 (original)
<https://reviews.apache.org/r/59508/#comment252394>
Empty space/tab. This is causing the file to be shown in the patch even if it does nothing to do with the change.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 71 (patched)
<https://reviews.apache.org/r/59508/#comment252396>
I think this should be setHMSClient() as it is just doing that. Makes more sense I think.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 82 (patched)
<https://reviews.apache.org/r/59508/#comment252397>
Put a space between the operator 'return client != null' or you may use 'return (client != null)'. Typical coding conventions used in Apache projects is to have this space in the operators.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 206 (patched)
<https://reviews.apache.org/r/59508/#comment252398>
If the fetchFullUpdate() snapshot ignores the failure, why we have to throw an exception here?
This might confuse users when there are empty snapshots, right? Isn't better just to ignore the error and return an empty map list as well?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 251 (patched)
<https://reviews.apache.org/r/59508/#comment252399>
A typo on Get's. Should be Gets.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 260 (patched)
<https://reviews.apache.org/r/59508/#comment252400>
getFullSnapShot() throws an exception if the HMS is not connected, and this function just returns an empty list. We should be consistent about what to do when calling methods that require an HMS client connected.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 221 (original), 109 (patched)
<https://reviews.apache.org/r/59508/#comment252401>
Is this method needed now? Seems it is not used anywhere.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 533 (original), 224 (patched)
<https://reviews.apache.org/r/59508/#comment252402>
Shouldn't this condition happen early? Perhaps after checking if events.isEmpty()? Seems odd that we might read the hive-site.xml, instatiate the notificationProcessor, and then exit on the first
event because the leaderMonitor is not the leader.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 648 (original), 270 (patched)
<https://reviews.apache.org/r/59508/#comment252403>
Couldn't be this a IOException? It is attempting to persist (write/output) to a DB, so it looks like IOException make sense.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 288 (patched)
<https://reviews.apache.org/r/59508/#comment252404>
Couldn't be this a IOException? It is attempting to persist (write/output) to a DB, so it looks like IOException make sense.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
Lines 20 (patched)
<https://reviews.apache.org/r/59508/#comment252405>
Seems the naming convention for Sentry exceptions are that classes start with Sentry###Exception. Should we follow the same idea? Also, why not having a generic exception instead of two (HMSFollowerPersistenceException and HMSFollowerSnapShotCreatingException)? Both refer to problems with the HMSFollower, and they only wrap
a message. Btw, is there a common Sentry###Exception we could use instead of creating an new one?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 75 (patched)
<https://reviews.apache.org/r/59508/#comment252406>
Need a space.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 85 (patched)
<https://reviews.apache.org/r/59508/#comment252407>
Need to fix indentation.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 96 (patched)
<https://reviews.apache.org/r/59508/#comment252408>
Remove space before the parethesis. "setAuthzConf (Configuration conf)"
Btw, if this method exists now, should we call it from the constructor? Seems we are duplicating code here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 148 (patched)
<https://reviews.apache.org/r/59508/#comment252409>
Shouldn't this be a warning instead of error? Error implies that something bad happened on the code. Here, we're just ignoring these invalid requests. A warning is more appropiate.
The same for the rest of the notifications requests.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java
Lines 37 (patched)
<https://reviews.apache.org/r/59508/#comment252410>
Need a space here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 233 (patched)
<https://reviews.apache.org/r/59508/#comment252395>
What's wrong with the the old getAuthzObj implementation? The old method was returning NULL instead of throwing an exception, something that is less expensive if it occurs very often. Also, it makes sense to return a NULL if the authzble object has as NULL db, right? The caller should handle the NULL value, and decide what to do with it instead of catching an exception.
- Sergio Pena
On June 20, 2017, 4:11 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 20, 2017, 4:11 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/4/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179707
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Line 41 (original), 41 (patched)
<https://reviews.apache.org/r/59508/#comment254542>
Can you include the test case "testCreateTableAfterInvalidEvent() throws Exception" in https://reviews.apache.org/r/59058/diff/3#index_header?
- Na Li
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179706
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 387 (original), 190 (patched)
<https://reviews.apache.org/r/59508/#comment254541>
We should log the first notification ID in the list, and avoid printing the same info multiple times, i.e. keep the behavior of SENTRY-1674: HMSFollower shouldn't print the same value of notification ID multiple times
- Na Li
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On June 30, 2017, 9:51 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java
> > Lines 26 (patched)
> > <https://reviews.apache.org/r/59508/diff/6/?file=1767658#file1767658line26>
> >
> > This looks pretty similar to the PathsImage class. In HIVE-1815, this class will include the image # as well. Should we use the PathsImage instead? The name makes a lot of sense too.
I did not realize that. They are same. I will make the change.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179412
-----------------------------------------------------------
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179412
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java
Lines 26 (patched)
<https://reviews.apache.org/r/59508/#comment254104>
This looks pretty similar to the PathsImage class. In HIVE-1815, this class will include the image # as well. Should we use the PathsImage instead? The name makes a lot of sense too.
- Sergio Pena
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179701
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 265 (patched)
<https://reviews.apache.org/r/59508/#comment254520>
We can remove this comment, and simplify the code accordingly.
As we discussed, you only need to call getNextNotification(), no need to call getCurrentNotificationEventId(). If exception happens, catch it and log. It is most likely the notification list is empty.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 124 (original), 80 (patched)
<https://reviews.apache.org/r/59508/#comment254538>
do you need to set "client = null"?
- Na Li
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 84 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line84>
> >
> > Is it an attempt to fix the problem with missing server name? Are you sure that this will have the setting?
> >
> > If you still use, please introduce a new variable rather then reusing the parameter.
>
> kalyan kumar kalvagadda wrote:
> No it is not. lina is looking into it.
I am assuming you revert to using Sentry config for this.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 105 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line105>
> >
> > It would be good to set client to null as well to allow multiple calls to close()
>
> kalyan kumar kalvagadda wrote:
> I don't see a reason to destoy SentryHmsClient. It is constructed when HMSFollower is constructed and stays. Based on situation close/connect are called on it.
I see how it works now. Dropping the issue.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 124 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line124>
> >
> > This doesn't look right. Once we become a leader, who will re-open the client?
>
> kalyan kumar kalvagadda wrote:
> On becoming leader, SentryHmsClient would open new connection using hiveConnectionFactory.
I see. I was confused because usually once you call close() you are not supposed to use an object. May be it is better to have a different method for client for this purpose.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 200 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line200>
> >
> > Why are we doing this - why not let the caller to log the failure? This will allow us to remove the whole try-block
>
> kalyan kumar kalvagadda wrote:
> if the caller is not doing anything specific to that excpetion we need not have handlers for those specific exceptions just to log the failure.
>
> With the approach i have taken, caller doesn't have any spefic handlers for these exception and all of then are handled with one block.
But the caller logs the exception anyway. Here you are not adding any extra value - just loggin an exception and passing it on.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 232 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line232>
> >
> > this is not needed since it is initialized to false initially
>
> kalyan kumar kalvagadda wrote:
> we need it as the isNotificationProcessed can be set to true in the loop.
> let's say a case where notification-1 is processed. isNotificationProcessed will be set to true.
> notification-2 is not processed becuase of an exception we need to explicitly set the isNotificationProcessed to false.
Looks like it should be set to false on each iteration?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180338
-----------------------------------------------------------
On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 5:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
>
>
> Diff: https://reviews.apache.org/r/59508/diff/9/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 84 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line84>
> >
> > Is it an attempt to fix the problem with missing server name? Are you sure that this will have the setting?
> >
> > If you still use, please introduce a new variable rather then reusing the parameter.
No it is not. lina is looking into it.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 105 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line105>
> >
> > It would be good to set client to null as well to allow multiple calls to close()
I don't see a reason to destoy SentryHmsClient. It is constructed when HMSFollower is constructed and stays. Based on situation close/connect are called on it.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 124 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line124>
> >
> > This doesn't look right. Once we become a leader, who will re-open the client?
On becoming leader, SentryHmsClient would open new connection using hiveConnectionFactory.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 141 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line141>
> >
> > Do we need to do it here? Can we hie this check in the client instead so that we connect as needed before each peration on the client?
will remove
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 178 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line178>
> >
> > Why are we printing it to stdout rather then to the log?
will Change.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 200 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line200>
> >
> > Why are we doing this - why not let the caller to log the failure? This will allow us to remove the whole try-block
if the caller is not doing anything specific to that excpetion we need not have handlers for those specific exceptions just to log the failure.
With the approach i have taken, caller doesn't have any spefic handlers for these exception and all of then are handled with one block.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 213 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line213>
> >
> > and wake up any waitng clients
WIll fix
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 220 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line220>
> >
> > initialize it to false
I removed the initialization to handle the code style errors. Will fix
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 232 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line232>
> >
> > this is not needed since it is initialized to false initially
we need it as the isNotificationProcessed can be set to true in the loop.
let's say a case where notification-1 is processed. isNotificationProcessed will be set to true.
notification-2 is not processed becuase of an exception we need to explicitly set the isNotificationProcessed to false.
> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
> > Lines 255 (patched)
> > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line255>
> >
> > Why are we doing this here instead of the caller logging the exception? Since you just re-throw the exception you can as well just remove the try statement and propagate it to the caller
if the caller is not doing anything specific to that excpetion we need not have handlers for those specific exceptions just to log the failure.
With the approach i have taken, caller doesn't have any spefic handlers for these exception and all of then are handled with one block.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180338
-----------------------------------------------------------
On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 5:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
>
>
> Diff: https://reviews.apache.org/r/59508/diff/9/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180338
-----------------------------------------------------------
Comments for the HmsFollower class only. Not complete review.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 64 (patched)
<https://reviews.apache.org/r/59508/#comment255445>
It is ok, but a bit weird - the main constructor is test-only, not another way around.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 82 (patched)
<https://reviews.apache.org/r/59508/#comment255448>
This should be moved to the `if` case below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 84 (patched)
<https://reviews.apache.org/r/59508/#comment255446>
Is it an attempt to fix the problem with missing server name? Are you sure that this will have the setting?
If you still use, please introduce a new variable rather then reusing the parameter.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 88 (patched)
<https://reviews.apache.org/r/59508/#comment255449>
Extra line
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 103 (patched)
<https://reviews.apache.org/r/59508/#comment255451>
please also include the failure
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 105 (patched)
<https://reviews.apache.org/r/59508/#comment255450>
It would be good to set client to null as well to allow multiple calls to close()
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 124 (patched)
<https://reviews.apache.org/r/59508/#comment255452>
This doesn't look right. Once we become a leader, who will re-open the client?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 141 (patched)
<https://reviews.apache.org/r/59508/#comment255454>
Do we need to do it here? Can we hie this check in the client instead so that we connect as needed before each peration on the client?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 178 (patched)
<https://reviews.apache.org/r/59508/#comment255455>
Why are we printing it to stdout rather then to the log?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 200 (patched)
<https://reviews.apache.org/r/59508/#comment255457>
Why are we doing this - why not let the caller to log the failure? This will allow us to remove the whole try-block
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 213 (patched)
<https://reviews.apache.org/r/59508/#comment255458>
and wake up any waitng clients
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 220 (patched)
<https://reviews.apache.org/r/59508/#comment255459>
initialize it to false
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 232 (patched)
<https://reviews.apache.org/r/59508/#comment255460>
this is not needed since it is initialized to false initially
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 255 (patched)
<https://reviews.apache.org/r/59508/#comment255461>
Why are we doing this here instead of the caller logging the exception? Since you just re-throw the exception you can as well just remove the try statement and propagate it to the caller
- Alexander Kolbasov
On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 5:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
>
>
> Diff: https://reviews.apache.org/r/59508/diff/9/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated July 13, 2017, 4:26 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
addressed comments and rebsed the patch.
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 979e45b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1b6ae18
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6014a79
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 6e22875
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 51f6c5d
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
Diff: https://reviews.apache.org/r/59508/diff/10/
Changes: https://reviews.apache.org/r/59508/diff/9-10/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180392
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 84 (patched)
<https://reviews.apache.org/r/59508/#comment255544>
authServerName should be read from sentry-site.xml (its content is in conf at input), not hive-site.xml.
So you should not call "new HiveConf".
- Na Li
On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 5:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
>
>
> Diff: https://reviews.apache.org/r/59508/diff/9/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180363
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java
Lines 191-194 (patched)
<https://reviews.apache.org/r/59508/#comment255516>
Based on a comment I left on the TestSentryHmsClient class, I think we can put the before/after ID check here.
The below code keeping the HMS implementation on the SentryHmsClient, and what we want to do with the results on the caller.
createFullSnapshot() {
long idBeforeSnapshot = client.getNotificationId();
PathsImage snapshot = client.getFullSnapshot();
if (snapshot.isEmpty()) {
return SentryStore.EMPTY_PATHS_SNAPSHOT_ID;
}
long idAfterSnapshot = client.getNotificationId();
if (idBeforeSnapshot != idAfterSnapshot) {
LOGGER.info("different notifications ids");
return SentryStore.EMPTY_PATHS_SNAPSHOT_ID;
}
...
}
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java
Lines 154-156 (patched)
<https://reviews.apache.org/r/59508/#comment255514>
I still not convinced about this testing. This does not avoid that getFullSnapshot() may connect automatically to the HMS and get an empty snapshot or another kind of error ocurred.
The call to getFullSnapshot() when a client is not connected, in my opinion should throw an error. This error should never happen from HmsFollower because we're making sure that the connection happens before, but a good class design should let users know about these errors.
A better design would be to move the condition for before/after notification ID to the createFullSnapshot() method. This way we could test here that getFullSnapshot() would fail if no client is connected or the snapshot is empty or not when client.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java
Lines 165-169 (patched)
<https://reviews.apache.org/r/59508/#comment255515>
Same comment as testSnapshotCreationWithOutclientConnected()
- Sergio Pena
On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 5:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
>
>
> Diff: https://reviews.apache.org/r/59508/diff/9/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180430
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 5:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
>
>
> Diff: https://reviews.apache.org/r/59508/diff/9/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated July 12, 2017, 5:44 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Rebased the patch.
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java 2426b40
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java d6100de
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 5b8a572
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java d683c2c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java 4d852e6
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java ad23334
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 2d581f7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHmsClient.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHmsFollower.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHmsClient.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java b9330cc
Diff: https://reviews.apache.org/r/59508/diff/9/
Changes: https://reviews.apache.org/r/59508/diff/8-9/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
> On July 10, 2017, 6:31 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 627 (original), 213 (patched)
> > <https://reviews.apache.org/r/59508/diff/8/?file=1772851#file1772851line653>
> >
> > Hmm - this look svery strange. Why do we only call `persistLastProcessedNotificationID` when notification is *not* process? Shouldn't we call it as well when notification *is* processed?
If the event is processed, the notification ID is automatically persisted. Only when it is not processed correctly (skipped), we need to explicitly save it.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180042
-----------------------------------------------------------
On July 10, 2017, 3:14 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 10, 2017, 3:14 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java de94743
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java fd56ce2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/8/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review180042
-----------------------------------------------------------
I still see a lot of issues with code style even after you applied formatting to some files.
For tests, especially new ones, please document what test cases are actually testing.
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 253 (patched)
<https://reviews.apache.org/r/59508/#comment255040>
What is the reason for changing SentryInvalidInputException to SentryPluginException?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 57 (patched)
<https://reviews.apache.org/r/59508/#comment255042>
hiveServerName is completely confusing name for this variable. Also, please add comment explaining what that this server name actually means.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 96 (original), 62 (patched)
<https://reviews.apache.org/r/59508/#comment255041>
It would be nice to refctor test constructor to call real constructor instead of duplicating code.
Also, please document both constructors, especially test-only constructor.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 124 (original), 80 (patched)
<https://reviews.apache.org/r/59508/#comment255051>
Do we want to set client to null as well?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 364 (original), 167 (patched)
<https://reviews.apache.org/r/59508/#comment255050>
Interesting - this is done as two separate transactions, so it is possible to write snapshot but fail writing latest snapshot ID. this seems wrong. Do we have existing JIRA filed for this problem?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 627 (original), 213 (patched)
<https://reviews.apache.org/r/59508/#comment255056>
Hmm - this look svery strange. Why do we only call `persistLastProcessedNotificationID` when notification is *not* process? Shouldn't we call it as well when notification *is* processed?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 232 (patched)
<https://reviews.apache.org/r/59508/#comment255054>
What is the point in having this "wrapper"? Seems rather useless.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 253 (patched)
<https://reviews.apache.org/r/59508/#comment255055>
What is the point in having this "wrapper"? Seems rather useless.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 34 (original), 58 (patched)
<https://reviews.apache.org/r/59508/#comment255059>
applies *to*
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 46 (original), 75 (patched)
<https://reviews.apache.org/r/59508/#comment255060>
Please add javadoc.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 101 (patched)
<https://reviews.apache.org/r/59508/#comment255063>
This doc isn't very useful and doesn't really reflect/explain what the code is doing.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 113 (patched)
<https://reviews.apache.org/r/59508/#comment255064>
according to javadoc this method is the same as the previous but it isn't.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 54 (patched)
<https://reviews.apache.org/r/59508/#comment255066>
Should it implement AutoCloseable?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 71 (patched)
<https://reviews.apache.org/r/59508/#comment255068>
Do we actually need this? We can have a test-only constructor that accepts the client as parameter
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 132 (patched)
<https://reviews.apache.org/r/59508/#comment255071>
We don't need to do this on every connect - this can be done just once. But we can fix this separately from this refactoring.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Line 47 (original), 57 (patched)
<https://reviews.apache.org/r/59508/#comment255073>
Please document what this and other tests are actually testing.
- Alexander Kolbasov
On July 10, 2017, 3:14 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 10, 2017, 3:14 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java de94743
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java fd56ce2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/8/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated July 10, 2017, 3:14 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Applied goolge code style on the files that are effected by this refactoring. Also addressed review comments.
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java de94743
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java fd56ce2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
Diff: https://reviews.apache.org/r/59508/diff/8/
Changes: https://reviews.apache.org/r/59508/diff/7-8/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On July 7, 2017, 3:22 p.m., Alexander Kolbasov wrote:
> > The latest changeset is missing HMSClientWrapper - seems like it isn't correct, please verify.
It is not missing. I have renamed it to SentryHMSClient.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179875
-----------------------------------------------------------
On July 6, 2017, 3:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 3:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/7/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179875
-----------------------------------------------------------
The latest changeset is missing HMSClientWrapper - seems like it isn't correct, please verify.
- Alexander Kolbasov
On July 6, 2017, 3:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 3:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/7/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179881
-----------------------------------------------------------
Initial review - it isn't complete yet.
Please run the files through checkstye.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java
Lines 24 (patched)
<https://reviews.apache.org/r/59508/#comment254779>
Full snapshot of what? And what is notification id corresponding to that snapshot? The reader needs a lot of context to understand what this class is about.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 18 (patched)
<https://reviews.apache.org/r/59508/#comment254894>
Style - add a newline here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 290 (patched)
<https://reviews.apache.org/r/59508/#comment254908>
I think
return Collections.emptyList();
would work here.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
Lines 55 (patched)
<https://reviews.apache.org/r/59508/#comment254909>
This and the next one an be final
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
Lines 58 (patched)
<https://reviews.apache.org/r/59508/#comment254912>
Can you share these with testFullUpdateInitializer?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
Lines 62 (patched)
<https://reviews.apache.org/r/59508/#comment254910>
This and the next one can be private. They can be final as well.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
Lines 70 (patched)
<https://reviews.apache.org/r/59508/#comment254911>
This one is never used
- Alexander Kolbasov
On July 6, 2017, 3:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 3:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/7/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated July 6, 2017, 3:44 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
ADDRESSED REVIEW COMMENTS
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java PRE-CREATION
Diff: https://reviews.apache.org/r/59508/diff/7/
Changes: https://reviews.apache.org/r/59508/diff/6-7/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review179702
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 259 (patched)
<https://reviews.apache.org/r/59508/#comment254522>
Sorry to bother you about the name, but the HMS word is already implicit on the class name. getNewNotifications() or just getNotifications() might make more sense.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 385 (original), 188 (patched)
<https://reviews.apache.org/r/59508/#comment254521>
Why is this public?. Keep it package-private so that it can be used by TestHMSFollower.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 52 (patched)
<https://reviews.apache.org/r/59508/#comment254523>
I recommened using better names for the methods instead of comments. The method name appears on the JUnit report, and in Jenkins, and in the maven output whenever they fail, so it is quicker to know what test case fail instead of just saying testFailure1().
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 237 (patched)
<https://reviews.apache.org/r/59508/#comment254524>
I don't think we should use random numbers in unit tests. This could cause that getCreateDatabaseNotification() returns a new notification with the same ID if it is not handled well. I prefer ti pass the db name as a parameter instead.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 250 (patched)
<https://reviews.apache.org/r/59508/#comment254525>
Same issue as line 237. Let's pass the db name as a parameter instead of creating a randome one.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 268 (patched)
<https://reviews.apache.org/r/59508/#comment254526>
How do you know this call is failing? The \ getFullSnapshot() method returns an empty snapshot for different situations.
- HMS client is not initialized
- Snapshot fetched is empty
- Notifications ID before/after are different
- An exception happened while fetching snapshots
We should define the behavior of getFullSnapshot() that let us understand what happens.
- HMS client is not initialized
Should we throw an exception if attempting to call this method when the HMS client is not initialized?
IllegalStateException is a good one. We shouldn't call this method when the HMS client is not initialized.
- Snapshot fetched is empty
Current behavior is ok.
- Notifications ID before/after are different
Should we treat this as an error for the caller?
Would a generic Exception() work?
- An exception happened while fetching snapshots
Should we throw the exception caught?
The above definitions will let us validate the correct behavior of the getFullSnapshot() method.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 273 (patched)
<https://reviews.apache.org/r/59508/#comment254527>
'there HMS doesn't' seems a typo on the comment?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 278 (patched)
<https://reviews.apache.org/r/59508/#comment254528>
Same issue as line 268. The getFullSnapshot() gets empty snapshots for different situations. This does not validate that the method had a failure or just returned empty data.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 311 (patched)
<https://reviews.apache.org/r/59508/#comment254529>
Same issue as line 268. The getFullSnapshot() gets empty snapshots for different situations. This does not validate that the method had a failure, returned just empty data or after/before notifications are different.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 342 (patched)
<https://reviews.apache.org/r/59508/#comment254530>
Can this happen? You have a control workflow when calling the getFullSnapshot() method. You have mocks that does no throw exceptions, so how can this fail?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 365 (patched)
<https://reviews.apache.org/r/59508/#comment254531>
Same issue as line 268. How do I know that the method caused an error or probably it worked but returned an empty snapshot due a bug in the method? It's better to define some behavior and results for those situations.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 389 (patched)
<https://reviews.apache.org/r/59508/#comment254532>
Same issue as line 268.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java
Lines 406 (patched)
<https://reviews.apache.org/r/59508/#comment254533>
I don't see the Hivesnapshot being used inside the getHMSNotifications() method. Why do we need it?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Line 41 (original), 41 (patched)
<https://reviews.apache.org/r/59508/#comment254537>
This test class is having the same issue as SENTRY-1775 (Unit test for HMSFollower is not testing anything useful). Are you going create test cases on SENTRY-1775 instead?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java
Lines 47 (patched)
<https://reviews.apache.org/r/59508/#comment254535>
For all the verify() calls we're doing, we should check that the parameters are the ones we expect and not just any values.
This allows the unit test to fail in case the method sends invalid values to the sentryStore. This will catch bugs later if someone make changes.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java
Lines 96 (patched)
<https://reviews.apache.org/r/59508/#comment254536>
Shouldn't we verify that the parameters passed to the addAuthzPathsMapping() are the ones we want? This will validate that the method is working correctly and not just calling the methods with invalid parameters.
- Sergio Pena
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated June 30, 2017, 4:41 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed the review comments
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
Diff: https://reviews.apache.org/r/59508/diff/6/
Changes: https://reviews.apache.org/r/59508/diff/5-6/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178922
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 255 (patched)
<https://reviews.apache.org/r/59508/#comment253322>
just use failure, drop failure,.toString()
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 69 (patched)
<https://reviews.apache.org/r/59508/#comment253286>
This doesn't initialize anything - it is a simple setter, so probably should be just setClient()
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 104 (patched)
<https://reviews.apache.org/r/59508/#comment253287>
We can return immediately here
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 188 (patched)
<https://reviews.apache.org/r/59508/#comment253249>
Signature shows that it throws Exception, it is inconsistent with the doc
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 190 (patched)
<https://reviews.apache.org/r/59508/#comment253248>
Snapshot is a single word
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 210 (patched)
<https://reviews.apache.org/r/59508/#comment253288>
Should we log something in this case as well?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 211 (patched)
<https://reviews.apache.org/r/59508/#comment253284>
String.format doesn't understand {} format specifiers, only log4j does
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 264 (patched)
<https://reviews.apache.org/r/59508/#comment253290>
should be
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 267 (patched)
<https://reviews.apache.org/r/59508/#comment253292>
else is not needed since the previous clause ends with return
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 273 (patched)
<https://reviews.apache.org/r/59508/#comment253295>
This isn't needed - if it is empty, we return an empty collection anyway.
So it can be just
return response.isSetEvents() ? response.getEvents() : Collections.<NotificationEvent>emptyList();
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 275 (patched)
<https://reviews.apache.org/r/59508/#comment253294>
we are not processing anything here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 84 (original), 48 (patched)
<https://reviews.apache.org/r/59508/#comment253311>
This isn't hiveServerName, but location(s) of HMS service, so a better name can be hmsLocation.
Please add comment explaining why it isn't final since it is very non-trivial.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 221 (original), 110 (patched)
<https://reviews.apache.org/r/59508/#comment253312>
IntelliJ shows this as not used
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 251 (original), 130 (patched)
<https://reviews.apache.org/r/59508/#comment253313>
shouldn't we set client to null in this case?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 258 (original), 136 (patched)
<https://reviews.apache.org/r/59508/#comment253315>
This is rather strange function - we can decide whether it is needed or not and have a function that just returns a full snapshot unconditionally
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 336 (original), 138 (patched)
<https://reviews.apache.org/r/59508/#comment253314>
Is it always TException? Can it throw some kind of Kerberos exception?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 358 (original), 167 (patched)
<https://reviews.apache.org/r/59508/#comment253316>
here and below, snapshot is a single word
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 385 (original), 193 (patched)
<https://reviews.apache.org/r/59508/#comment253317>
Why do we need this function? It isn't called by anything else, so we can just inline it as
processNotifications(client.getHMSNotifications(
notificationID))
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 219 (patched)
<https://reviews.apache.org/r/59508/#comment253318>
Why do we need to create a new NotificationProcessor every time? Why not just create one in constructor?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 622 (original), 235 (patched)
<https://reviews.apache.org/r/59508/#comment253319>
We are throwing it only to immediately catch in the caller and print an error message - what is the point?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 648 (original), 270 (patched)
<https://reviews.apache.org/r/59508/#comment253320>
Again, we are throwing this exception only to catch it in the same class in another method and pint an error - we can as well do it here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 288 (patched)
<https://reviews.apache.org/r/59508/#comment253321>
Same comment about throwing exception
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
Lines 20 (patched)
<https://reviews.apache.org/r/59508/#comment253247>
I still do not see a value in the special exception here - it is only used to show different logs which can be achieved without introducing special exceptions.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 83 (patched)
<https://reviews.apache.org/r/59508/#comment253298>
why not just syncOnCreate?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 95 (patched)
<https://reviews.apache.org/r/59508/#comment253299>
I thinuk it is better to have explicit setters for these variables rather then pass this via conf
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 97 (patched)
<https://reviews.apache.org/r/59508/#comment253300>
Why this is not done the same way as in constructor?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 113 (patched)
<https://reviews.apache.org/r/59508/#comment253301>
can this be package-private?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 130 (patched)
<https://reviews.apache.org/r/59508/#comment253304>
Add case INSERT: with whatever shopuld happen there (possibly just return true)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 132 (patched)
<https://reviews.apache.org/r/59508/#comment253305>
Log an error because this should never happen once you handle INSERT
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 241 (original), 455 (patched)
<https://reviews.apache.org/r/59508/#comment253306>
Remove these hashes at the beginning
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 303 (original), 517 (patched)
<https://reviews.apache.org/r/59508/#comment253307>
Remove hashes at the beginning
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 394 (original), 609 (patched)
<https://reviews.apache.org/r/59508/#comment253308>
This should be static (as well as getPath()
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 654 (patched)
<https://reviews.apache.org/r/59508/#comment253309>
What is this function doing?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 663 (patched)
<https://reviews.apache.org/r/59508/#comment253310>
What is this function doing?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java
Lines 17 (patched)
<https://reviews.apache.org/r/59508/#comment253285>
I don't think this exception is needed - no one even catches it explicitely!
- Alexander Kolbasov
On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/5/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated June 22, 2017, 8:28 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed comments from sergio and sasha
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
Diff: https://reviews.apache.org/r/59508/diff/5/
Changes: https://reviews.apache.org/r/59508/diff/4-5/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On June 21, 2017, 11:05 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Line 43 (original), 68 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line68>
> >
> > Why do we handle LOGGER in such a way here rather then creating the per-class logger as is done by everyone else?
>
> kalyan kumar kalvagadda wrote:
> I thought for some one trying to debug HMSFolower, they need not enable logging for a bunch of classes. Instead they can enabled it for HMSFOllower class and get logging for every thing.
In this case the same model should be followed by HMSClientWrapper. If you think that this is a valid reason, let's be consistent (and add comment). Otherwise just use normall logging as is used everywhere else.
> On June 21, 2017, 11:05 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Lines 111 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line111>
> >
> > Can it return false and not throw an exception? What is the difference between returning false and throwing exception?
>
> kalyan kumar kalvagadda wrote:
> Returning false is not complaining. It is just that the event is not processed to completion what ever reason. Exceptions are thown when there are issues while processing the exceptions. Different exceptions are dealt differently.
Please document this in javadoc - it isn't clear.
> On June 21, 2017, 11:05 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Lines 131 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line131>
> >
> > There should be INSERT case (which may do nothing) and the default should compain because it should never happen.
>
> kalyan kumar kalvagadda wrote:
> Returning false is not complaining. It is just that the event is not persisted. When we are processing INSERT event type why should we just handle and return false?
We should cover all possible case values. INSERT is one of them. In this case we should never fall in the default clause.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178576
-----------------------------------------------------------
On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/5/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178576
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 256 (patched)
<https://reviews.apache.org/r/59508/#comment252673>
USe build-in formatting
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 326 (patched)
<https://reviews.apache.org/r/59508/#comment252674>
Use built-in formatting
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 41 (original), 66 (patched)
<https://reviews.apache.org/r/59508/#comment252642>
Why is it public?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 43 (original), 68 (patched)
<https://reviews.apache.org/r/59508/#comment252643>
Why do we handle LOGGER in such a way here rather then creating the per-class logger as is done by everyone else?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 73 (patched)
<https://reviews.apache.org/r/59508/#comment252646>
Please add comment explaining why this and the next one are not final.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 75 (patched)
<https://reviews.apache.org/r/59508/#comment252647>
new line
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 82 (patched)
<https://reviews.apache.org/r/59508/#comment252645>
It i sbetter to use Boolean.parseBoolean() innstead.
Also, naming - 'shouldPolicyStoreBeSyncedOnCreate' is way to long to comprehend.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 95 (patched)
<https://reviews.apache.org/r/59508/#comment252649>
Do you really need to pass config or just required values for sync params?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 96 (patched)
<https://reviews.apache.org/r/59508/#comment252648>
Since this is for testing only, should it be protected or package-private?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 107 (patched)
<https://reviews.apache.org/r/59508/#comment252650>
and persist to
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 111 (patched)
<https://reviews.apache.org/r/59508/#comment252651>
Can it return false and not throw an exception? What is the difference between returning false and throwing exception?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 131 (patched)
<https://reviews.apache.org/r/59508/#comment252669>
There should be INSERT case (which may do nothing) and the default should compain because it should never happen.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 226 (patched)
<https://reviews.apache.org/r/59508/#comment252670>
It is very specific about the way it gets the name, so the doc should explain how the name is obtained from the object.
- Alexander Kolbasov
On June 20, 2017, 4:11 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 20, 2017, 4:11 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/4/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On June 22, 2017, 8:02 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Lines 276 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754610#file1754610line291>
> >
> > return else part since previous part ends with return
>
> kalyan kumar kalvagadda wrote:
> I did not quite get it.
if (someCondition)
{
...
return false;
} else if ((oldDbName.equals(newDbName))
Since there is a return, else clause is not needed. so it can be just
if (someCOndition) {
...
return false;
}
if (oldDbName.equals(newDbName)) {
...
}
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178636
-----------------------------------------------------------
On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/5/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178636
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 41 (original), 66 (patched)
<https://reviews.apache.org/r/59508/#comment252783>
Should it be final?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 46 (original), 71 (patched)
<https://reviews.apache.org/r/59508/#comment252786>
This string has nothing to do with the hive instance and naming it this way is very confusing.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 148 (patched)
<https://reviews.apache.org/r/59508/#comment252788>
can we use built-in formatting instead of using String.format?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 91 (original), 210 (patched)
<https://reviews.apache.org/r/59508/#comment252789>
Didn't you introduce a new function for that?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Line 107 (original), 238 (patched)
<https://reviews.apache.org/r/59508/#comment252790>
You introduced a new function for that. I dont think it is really needed, but if you have it, why not use it?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 276 (patched)
<https://reviews.apache.org/r/59508/#comment252792>
return else part since previous part ends with return
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 290 (patched)
<https://reviews.apache.org/r/59508/#comment252794>
do not use %s, use {} instead
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 293 (patched)
<https://reviews.apache.org/r/59508/#comment252793>
use {} formatting instead of string operations
- Alexander Kolbasov
On June 20, 2017, 4:11 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 20, 2017, 4:11 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/4/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 92 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line92>
> >
> > There are several exceptions listed below - should they be mentioned here as well?
>
> kalyan kumar kalvagadda wrote:
> All the exception are listed here. Ami missing something.
The call has
void connect()
throws IOException, InterruptedException, LoginException, MetaException {
The comment only lists MetaException
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 174 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line174>
> >
> > This is a problem with the original code - if client.close() throws an exception, the kerberos context will never be shut down.
>
> kalyan kumar kalvagadda wrote:
> close on client doesn't throw an exception. Do you think it is an issue even then?
Hopefully it doesn't but I wouldn't be surprised if transport close throws some unchecked exceptions.
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 198 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line198>
> >
> > Do we really need a custom exception here?
>
> kalyan kumar kalvagadda wrote:
> persistLastProcessedNotificationID and persistFullSnapShot should throw exceptions. If there are exceptions persisting them HMSFollower should not proceed futhrur.
> If we do not want to throw a custom exception we should the exception that they recived and the handle them in HMSFollower run where all the exceptions are handled as an unknown exception.
>
> I feel having custom xustom exception for spefix failure helps us handle them better.
What is the value in having separate exception for these two cases? We can'do anything particularly different in these cases. It would be useful it callers can respond differently which they can't do. We do log the reason anyway - so what value do you see with custom exceptions?
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 205 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line205>
> >
> > I don't think it can ever be null
>
> kalyan kumar kalvagadda wrote:
> There is no harm in making sure of it.
We are not checking everything for null. If we have a contract that specifies that the value can't be null there is no reason to check for it.
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
> > Lines 21 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754608#file1754608line21>
> >
> > Seems like other Sentry exceptions are using unique version IDs. I am not sure how they ar egenerated.
>
> kalyan kumar kalvagadda wrote:
> I have done some digging into it to understand why version ID is used. They are used when the exceptions are serialized and de-serialized. That is not the case here so i used 1;
That's Ok, but it is better to be consistent - we have some exceptions that are using real IDs and some that use 1.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178456
-----------------------------------------------------------
On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 8:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/5/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On June 21, 2017, 10:13 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 92 (patched)
> > <https://reviews.apache.org/r/59508/diff/4/?file=1754606#file1754606line92>
> >
> > There are several exceptions listed below - should they be mentioned here as well?
>
> kalyan kumar kalvagadda wrote:
> All the exception are listed here. Ami missing something.
>
> Alexander Kolbasov wrote:
> The call has
>
> void connect()
> throws IOException, InterruptedException, LoginException, MetaException {
>
> The comment only lists MetaException
Here is the comment. It lists all the exceptions
```
/**
* Connects to HMS by creating HiveMetaStoreClient
*
* @throws IOException
* @throws InterruptedException
* @throws LoginException if Kerberos context creation failed using Sentry's kerberos
* credentials
* @throws MetaException if there was a problem on creating an HMSClient
*/
```
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178456
-----------------------------------------------------------
On June 30, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullSnapshotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 9c3e485
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/6/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review178456
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java
Line 25 (original), 24 (patched)
<https://reviews.apache.org/r/59508/#comment252638>
Side note - since the rest is a plain comment (not Javadoc, the use of HTML formatting is completely useless.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 54 (patched)
<https://reviews.apache.org/r/59508/#comment252459>
should it be final?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 57 (patched)
<https://reviews.apache.org/r/59508/#comment252458>
Stale comment
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 70 (patched)
<https://reviews.apache.org/r/59508/#comment252460>
should it be protected or package-private?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 81 (patched)
<https://reviews.apache.org/r/59508/#comment252552>
This can be package-private
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 92 (patched)
<https://reviews.apache.org/r/59508/#comment252590>
There are several exceptions listed below - should they be mentioned here as well?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 94 (patched)
<https://reviews.apache.org/r/59508/#comment252554>
Naming - by definition this is the class talking to HMS, so a simple connect() name should be good enough.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 97 (patched)
<https://reviews.apache.org/r/59508/#comment252550>
Please declare one variable per line. ALso we only need them if kerberos enabled, so move these closer to the use.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 122 (patched)
<https://reviews.apache.org/r/59508/#comment252461>
use built-in formatting
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 125 (patched)
<https://reviews.apache.org/r/59508/#comment252462>
use built-in formatting for preconditions
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 131 (patched)
<https://reviews.apache.org/r/59508/#comment252463>
use built-in formatting
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 174 (patched)
<https://reviews.apache.org/r/59508/#comment252589>
This is a problem with the original code - if client.close() throws an exception, the kerberos context will never be shut down.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 190 (patched)
<https://reviews.apache.org/r/59508/#comment252626>
This doesn't match the signature.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 195 (patched)
<https://reviews.apache.org/r/59508/#comment252594>
Why do you need this? from the code it seems rather useless.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 198 (patched)
<https://reviews.apache.org/r/59508/#comment252608>
Do we really need a custom exception here?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 201 (patched)
<https://reviews.apache.org/r/59508/#comment252595>
Brig the declaration to where it is used and combine with assignment
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 205 (patched)
<https://reviews.apache.org/r/59508/#comment252604>
I don't think it can ever be null
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 211 (patched)
<https://reviews.apache.org/r/59508/#comment252605>
space before {}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 224 (patched)
<https://reviews.apache.org/r/59508/#comment252464>
use built-in formatting
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 251 (patched)
<https://reviews.apache.org/r/59508/#comment252627>
Even better, it should be "Return all HMS notifications ..."
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 253 (patched)
<https://reviews.apache.org/r/59508/#comment252628>
Naming - for the purpose of this function we don't care whether it is last or not, we are asked to return all notification from some specified initial value.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 258 (patched)
<https://reviews.apache.org/r/59508/#comment252629>
This formatting is unusual. Probably should be
Collection<NotificationEvent> getHMSNotifications(long lastProcessedNotificationID)
throws Exception {
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 260 (patched)
<https://reviews.apache.org/r/59508/#comment252630>
Also, if client isn't connected, there is no point on constantly throwing the same error message over and over.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 268 (patched)
<https://reviews.apache.org/r/59508/#comment252631>
This can be combined with previous message into one.
Otherwise you need to clarify what is the id received.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 269 (patched)
<https://reviews.apache.org/r/59508/#comment252632>
The case where they are equal means that there are no new events. But the case when this is smaller means that something strange is happening and warrants a warning.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 277 (patched)
<https://reviews.apache.org/r/59508/#comment252465>
use built-in formatting
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
Lines 21 (patched)
<https://reviews.apache.org/r/59508/#comment252639>
Seems like other Sentry exceptions are using unique version IDs. I am not sure how they ar egenerated.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java
Lines 24 (patched)
<https://reviews.apache.org/r/59508/#comment252640>
Why do we need this?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java
Lines 19 (patched)
<https://reviews.apache.org/r/59508/#comment252606>
Why do we need this? Also, if it is needed, we need a better name.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java
Lines 26 (patched)
<https://reviews.apache.org/r/59508/#comment252617>
1) The class can be final
2) Naming - the name is long and confusing. And Snapshot is not two words but one.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java
Lines 29 (patched)
<https://reviews.apache.org/r/59508/#comment252619>
This isn't last notification ID but notification ID associated with this particular ID, so this is just snapshot ID.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java
Lines 37 (patched)
<https://reviews.apache.org/r/59508/#comment252623>
Naming - getPathsSnapshot() is shorter
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java
Lines 41 (patched)
<https://reviews.apache.org/r/59508/#comment252624>
I'd suggest getId(). Can it be package-private?
- Alexander Kolbasov
On June 20, 2017, 4:11 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 20, 2017, 4:11 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59508/diff/4/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed. Added new classes to test new classes added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/
-----------------------------------------------------------
(Updated June 20, 2017, 4:11 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Added some more refactoring and addressed review comments. This patch also has new test classes to test new classes added.
Bugs: SENTRY-1769
https://issues.apache.org/jira/browse/SENTRY-1769
Repository: sentry
Description
-------
Things included in refactoring.
1. Moved the complete notification processing logic to notificationProcessor.
2. Added new class HMSFollowerHelper class which does
HMS Client creation
HMS Client closure
Creating and persisting the HMS snapshot
3. Misc cleanup
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8b19c88
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java 6762de7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSClientWrapper.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 66ad2a1
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java PRE-CREATION
Diff: https://reviews.apache.org/r/59508/diff/4/
Changes: https://reviews.apache.org/r/59508/diff/3-4/
Testing
-------
Made sure that all the tests around HMSFollower and SentryStore passed.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176851
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 372 (original), 175 (patched)
<https://reviews.apache.org/r/59508/#comment250323>
You got this value at line 125, why do you need to get it again?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 81 (patched)
<https://reviews.apache.org/r/59508/#comment250325>
Can you add comment on the meaning of the returned value? It indicates if the event has been processed successfully
- Na Li
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
> On June 1, 2017, 7:26 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
> > Lines 71 (patched)
> > <https://reviews.apache.org/r/59508/diff/3/?file=1738639#file1738639line71>
> >
> > Why do we have to call HiveConf if we are already getting conf?
I think the configuration from input is sentry configuration. The HiveConf() is to load hive configuration from file hive-site.xml.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176648
-----------------------------------------------------------
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176648
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 140 (original), 114 (patched)
<https://reviews.apache.org/r/59508/#comment250043>
Was thinking why don't we make the HMSClientWrapper singleton and we simply invoke HMSClientWrapper.getInstance(authzConf) to get the connection and use it?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 228 (original), 164 (patched)
<https://reviews.apache.org/r/59508/#comment250044>
nit. typo
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 209-212 (patched)
<https://reviews.apache.org/r/59508/#comment250045>
Aren't we already checking this at the beginning of every periodic execution of HMSFollower?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 69 (patched)
<https://reviews.apache.org/r/59508/#comment250041>
You are already passing the conf to the constructor. Let us simply save it in a variable and use it inside as a class variable.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 71 (patched)
<https://reviews.apache.org/r/59508/#comment250042>
Why do we have to call HiveConf if we are already getting conf?
- Vamsee Yarlagadda
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176831
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java
Line 24 (original)
<https://reviews.apache.org/r/59508/#comment250306>
Extra space. Please remove it from the patch.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 49 (patched)
<https://reviews.apache.org/r/59508/#comment250309>
The name does not sound correct now that I think of. HMSClientWrapper is not only wrapping HMS calls, it is also precessing notifications and persisting data on Sentry. Shouldn't be better to have a HiveSnapshotHelper instead?
class HiveSnapshotHelper {
/* save conf and sentrystore for reference */
public HiveSnapshotHelper(Configuration conf, SentryStore sentryStore);
/* fetch and persist a new snapshot; return true if persisted, false if not (throw exceptions on errors) */
public boolean createAndPersistSnapshot();
/* fetch and process new notifications; return true if persisted, false if not (throw exceptions on errors) */
public boolean processNewNotifications();
public void close();
}
Also, how can we re-connect to HMS if close() is called? I see that HMSFollower just creates another instance, but being a public class, shouldn't we have an better interface or contract to do that? probably having a connect()/isConnected() methods? or make the important methods to re-connect if it is not connected?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 54 (patched)
<https://reviews.apache.org/r/59508/#comment250308>
Add a space here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 190 (patched)
<https://reviews.apache.org/r/59508/#comment250310>
This isn't an error. An empty snapshot is a normal thing, why should we treat this as a failure?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 211 (patched)
<https://reviews.apache.org/r/59508/#comment250311>
Shouldn't we persist the latest notification ID in this method? Persisting a snapshot + notification ID are related, aren't they? If so, then the method should do it here instead of expecting the caller does
it. This would be useful for testing too, the tests should only call createAndPersistSnapshot() and just
check everything is correct.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 246 (original), 115 (patched)
<https://reviews.apache.org/r/59508/#comment250307>
Do we need this variable? Isn't enough to check if client != null to verify if the HMS client is connected?
- Sergio Pena
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59508: SENTRY-1769 Refactor HMSFollower Class
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59508/#review176848
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java
Lines 52 (patched)
<https://reviews.apache.org/r/59508/#comment250320>
The lastLoggedEventId should be notification ID, so its initial value should be EMPTY_NOTIFICATION_ID
- Na Li
On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59508/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 6:04 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1769
> https://issues.apache.org/jira/browse/SENTRY-1769
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Things included in refactoring.
> 1. Moved the complete notification processing logic to notificationProcessor.
> 2. Added new class HMSFollowerHelper class which does
> HMS Client creation
> HMS Client closure
> Creating and persisting the HMS snapshot
> 3. Misc cleanup
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 0bd0833
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 0d54548
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java cb05a84
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d410a6c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java de8e2f7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java 215f7d5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java 74a5afb
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java 1ed92ea
>
>
> Diff: https://reviews.apache.org/r/59508/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the tests around HMSFollower and SentryStore passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>