> On May 16, 2017, 6:54 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java > > Lines 47 (patched) > > <https://reviews.apache.org/r/59174/diff/3/?file=1718420#file1718420line47> > > > > 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.
This is not an e2e test. There are plans to come-up with such e2e tests after SENTRY-1708. > On May 16, 2017, 6:54 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java > > Lines 91 (patched) > > <https://reviews.apache.org/r/59174/diff/3/?file=1718420#file1718420line91> > > > > This variable is not used anywhere in the test. I will remove it. > On May 16, 2017, 6:54 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java > > Lines 105 (patched) > > <https://reviews.apache.org/r/59174/diff/3/?file=1718420#file1718420line105> > > > > 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. I will change the name and also add some comments. > On May 16, 2017, 6:54 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java > > Lines 134 (patched) > > <https://reviews.apache.org/r/59174/diff/3/?file=1718420#file1718420line134> > > > > 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? This is just a test right, I don;t think it really matters if Configuration and HMSFollower objects are created inside the thread context. Either way there will be only two threads. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59174/#review175134 ----------------------------------------------------------- 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 > >
