----------------------------------------------------------- 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 > >
