You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sergio Pena <se...@cloudera.com> on 2017/07/05 20:37:46 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/#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
> 
>