----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59174/#review175134 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java Lines 47 (patched) <https://reviews.apache.org/r/59174/#comment248513> Does this test belongs to a different suite of tests in sentry-tests? This test case tests more than a simple unit of work. It expects that a method does not fail when working concurrently and data is not duplidated on the DB. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java Lines 47 (patched) <https://reviews.apache.org/r/59174/#comment248514> Does this test belongs to a different suite of tests in sentry-tests? This test case tests more than a simple unit of work. It expects that a method does not fail when working concurrently and data is not duplidated on the DB. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java Lines 91 (patched) <https://reviews.apache.org/r/59174/#comment248515> This variable is not used anywhere in the test. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java Lines 91 (patched) <https://reviews.apache.org/r/59174/#comment248516> This variable is not used anywhere in the test. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java Lines 105 (patched) <https://reviews.apache.org/r/59174/#comment248507> This test case name sounds too general. There are at least two ways to test HMSFollower concurrency. This test is related to processing notifications only. Should we rename the test case to have a better idea of what it does? Perhaps something like testConcurrentProcessingNotificationsDoesNotDuplidateDAta? or something similar? Btw, I like the idea of naming test casses with this convention: testWhatToTestWhatToExpect(), like testProcessingInvalidNotificationsDoNotThrowExceptions(). This gives developers a very clear idea of what it is testing. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java Lines 134 (patched) <https://reviews.apache.org/r/59174/#comment248510> Should we create the Configuration and HMSFollower objects ouside the thread context? I believe that if we test concurrency, we should only call the method we want to test, this case is processNotificationEvents(), and avoid allocating resources previous to the test. In a real word scenario, only processNotificationEvents() is called at the same time. Also, should NUM_OF_FOLLOWERS be a local variable instead of a class variable? This test case specifically tests 2 concurrent threads, right?. Next test case might test 1 leader and 2 non-leader? - Sergio Pena On May 15, 2017, 4:30 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59174/ > ----------------------------------------------------------- > > (Updated May 15, 2017, 4:30 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, > Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1761 > https://issues.apache.org/jira/browse/SENTRY-1761 > > > Repository: sentry > > > Description > ------- > > Added test to make sure that SENTRY_HMS_NOTIFICATION_ID table is properly > updated even when multiple HMSFollowers are processing notifications from HMS. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59174/diff/3/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
