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/05/11 12:51:11 UTC
Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/
-----------------------------------------------------------
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/1/
Testing
-------
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On May 15, 2017, 4:21 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
> > Lines 92 (patched)
> > <https://reviews.apache.org/r/59174/diff/2/?file=1716959#file1716959line92>
> >
> > Need a space between methods.
will fix.
> On May 15, 2017, 4:21 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
> > Lines 102 (patched)
> > <https://reviews.apache.org/r/59174/diff/2/?file=1716959#file1716959line102>
> >
> > Need a space between methods.
will fix.
> On May 15, 2017, 4:21 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
> > Lines 134-135 (patched)
> > <https://reviews.apache.org/r/59174/diff/2/?file=1716959#file1716959line134>
> >
> > Are we testing the situation when two HMSFollower are processing notifications at the same time? Do we allow this situation?
This is not a desirable situation but it is possible. There should situataions where both the nodes assume to be leader and act accordingly. They may not be in this state for a long time tough.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review174959
-----------------------------------------------------------
On May 11, 2017, 12:51 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 12:51 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review174959
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 92 (patched)
<https://reviews.apache.org/r/59174/#comment248263>
Need a space between methods.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 102 (patched)
<https://reviews.apache.org/r/59174/#comment248264>
Need a space between methods.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 112 (patched)
<https://reviews.apache.org/r/59174/#comment248265>
Optional:
I've seen this line duplicated many times when creating events for testing. Should it be good to have an utility method that creates these events? Something like:
newDatabaseEvent(ID, new Database());
It may be in a common place for utilities classes for testing.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 134-135 (patched)
<https://reviews.apache.org/r/59174/#comment248269>
Are we testing the situation when two HMSFollower are processing notifications at the same time? Do we allow this situation?
- Sergio Pena
On May 11, 2017, 12:51 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 12:51 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On June 6, 2017, 12:07 a.m., Vamsee Yarlagadda 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/4/?file=1724141#file1724141line47>
> >
> > IS this really a concern anymore given there can be only one HMSFollower running?
There is no guarentee that there would be only one HMSFollower running. There could be certain time when both the sentry servers could be acting as leaders.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review176968
-----------------------------------------------------------
On May 18, 2017, 7:16 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 18, 2017, 7:16 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review176968
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 47 (patched)
<https://reviews.apache.org/r/59174/#comment250511>
IS this really a concern anymore given there can be only one HMSFollower running?
- Vamsee Yarlagadda
On May 18, 2017, 7:16 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 18, 2017, 7:16 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review175558
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 111 (patched)
<https://reviews.apache.org/r/59174/#comment248950>
All our unit test methods start with 'test...'. We should keep this convention as well.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 143 (patched)
<https://reviews.apache.org/r/59174/#comment248955>
I wonder why we need to test the whole processNotificationEvents() in parallel.
Are we trying to validate that sentryStore.persistLastProcessedNotificationID() can run in parallel? Why not run the tests on that method instead?
Or, why not running processNotificationEvents() twice in serial? That I think will do the same test if you want to validate that current data cannot be altered any more.
The HMSFollower.processNotificationEvents() has a lot of code that is not affected by parallelism. I think we should isolate the concurrent code and test only that if necessary.
Btw, doing tests with multi threads is a tough challenge.
- Sergio Pena
On May 18, 2017, 7:16 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 18, 2017, 7:16 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/
-----------------------------------------------------------
(Updated May 18, 2017, 7:16 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-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 (updated)
-----
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/4/
Changes: https://reviews.apache.org/r/59174/diff/3-4/
Testing
-------
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> 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
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review175019
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
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
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed review comments.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/59174/diff/2-3/
Testing
-------
Thanks,
kalyan kumar kalvagadda
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On May 12, 2017, 3:28 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
> > Lines 144 (patched)
> > <https://reviews.apache.org/r/59174/diff/1/?file=1715267#file1715267line144>
> >
> > waiting for 5000 seconds seems really long. are you sure the unit is seconds instead of ms?
It shouldn't be that long. I agree. Will fix it.
> On May 12, 2017, 3:28 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
> > Lines 149 (patched)
> > <https://reviews.apache.org/r/59174/diff/1/?file=1715267#file1715267line149>
> >
> > why do you need this?
Will remove.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review174798
-----------------------------------------------------------
On May 11, 2017, 12:51 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 12:51 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 59174: SENTRY-1761 Add test for concurrent HMS
followers processing notifications
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59174/#review174798
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 144 (patched)
<https://reviews.apache.org/r/59174/#comment248020>
waiting for 5000 seconds seems really long. are you sure the unit is seconds instead of ms?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestConcurrentHMSFollowers.java
Lines 149 (patched)
<https://reviews.apache.org/r/59174/#comment248021>
why do you need this?
- Na Li
On May 11, 2017, 12:51 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59174/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 12:51 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/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>