You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by Brian Towles <bt...@cloudera.com> on 2017/09/06 15:52:21 UTC

Re: Review Request 62107: Post review changes

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 6, 2017, 10:52 a.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Summary (updated)
-----------------

Post review changes


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description (updated)
-------

SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 


Diff: https://reviews.apache.org/r/62107/diff/4/

Changes: https://reviews.apache.org/r/62107/diff/3-4/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: Post review changes

Posted by Na Li <li...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184747
-----------------------------------------------------------




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 111 (original), 111 (patched)
<https://reviews.apache.org/r/62107/#comment260947>

    this check should be "if (seqNum > SentryStore.EMPTY_NOTIFICATION_ID && deltaRetriever.isDeltaAvailable(seqNum))" not ">=" because when seqNum is 0, sentry should return full snapshot, not delta


- Na Li


On Sept. 6, 2017, 3:52 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2017, 3:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected
> 
> Added fix to limit sending back of full snapshot when the HMSFollower is pulling a full snapshot
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/4/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.


> On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Lines 86-90 (patched)
> > <https://reviews.apache.org/r/62107/diff/3/?file=1816354#file1816354line91>
> >
> >     This code is repeated twice. Shouldn't be good to create a private method that retrieves the full image or an empty list if one is being loaded?

This is a good idea, we can leave the current logic and only return the full snapshot if the full update isnt being run.


> On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Lines 120 (patched)
> > <https://reviews.apache.org/r/62107/diff/3/?file=1816354#file1816354line125>
> >
> >     Do we need the 'seqNum <= SEQUENCE_NUMBER_UPDATE_UNINITIALIZED' condition?
> >     
> >     If an HDFS request comes in with a seqNum = 5, then we will send a new snapshot even if one is being loaded on the HMSFollower?

Not any more, per above.


> On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
> > Lines 67-69 (patched)
> > <https://reviews.apache.org/r/62107/diff/3/?file=1816355#file1816355line67>
> >
> >     Which code path we're executing here? The condition that appears on the start that checks if a snapshot is required is not executed:
> >     
> >         if (curImgNum > imgNum) {
> >             if (SentryServiceFactory.getInstance().isFullUpdateRunning()) {
> >               LOGGER.debug(
> >                   "A full update is being loaded. Delaying updating client with full image until its finished.");
> >               return Collections.emptyList();
> >             }
> >             // In case a new HMS snapshot has been processed, then return a full paths image.
> >             LOGGER.info("A newer full update is found with image number: {}", curImgNum);
> >             return Collections.singletonList(imageRetriever.retrieveFullImage());
> >           }
> >         
> >     curImgNum = 1
> >     imgNum = 1

Was if curImageNum == imgNum  we move on and are looking for deltas.  But per above reverting to old logic with seperate function for getting the full snapshot.


> On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 299 (patched)
> > <https://reviews.apache.org/r/62107/diff/3/?file=1816356#file1816356line299>
> >
> >     Is this assertion displayed on logs as an error?
> >     Could this error happen at some point? HMSFollower is not configured to run by several threads, I think we're safe, aren't we?

The second form of the assertion statement is:

assert Expression1 : Expression2 ;
where:

Expression1 is a boolean expression.
Expression2 is an expression that has a value. (It cannot be an invocation of a method that is declared void.)
Use this version of the assert statement to provide a detail message for the AssertionError. The system passes the value of Expression2 to the appropriate AssertionError constructor, which uses the string representation of the value as the error's detail message.


This was requested by Sasha's in previous reviews


> On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java
> > Line 25 (original), 33 (patched)
> > <https://reviews.apache.org/r/62107/diff/3/?file=1816358#file1816358line36>
> >
> >     Is the 'instance' object overriden if we call create() more than once?
> >     
> >     I've seen code where the create() or getInstance() checks if instance is null before creating the object, if it is not null, then it justs returns the instance without creating it again. Maybe we could merge create() and getInstance() in one method?

I was going to leave this for SENTRY-1921 but since we have a lot of questions about it I have just converted the SentryService itself to be a singleton and control the config.  This will remove the SentryServiceFactory and allow it to be all compartmentalized in the SentryService.


- Brian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184692
-----------------------------------------------------------


On Sept. 6, 2017, 6:33 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2017, 6:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Converted SentryService to a singleton and have HMSFollower setting a flag when a full update is running.  Set the DBUpdateForwarder to only send back full updates when then full update is not running.  Changed tests and added a Helper to support the new singleton.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java 4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java 8959ad8ec77a1268a755faf451d99a81d87cb889 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java 3b3b30e0bdf65e774d854e8252339960afcdd306 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java fe4164d4980738e8df6e93ee5f78e82e1d874ae1 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 32e67b9f8efbbec12e93794f0ab00d5b8ed555b1 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 6895720f6e6d5f12ce468d7368e0d0ee9a0fae88 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java b416ef81db21dfcf0157b2947c53721d5bcdf560 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 4cfb1f74cd90dbdb7c1aa5be07729e68353dd501 
>   sentry-tests/sentry-tests-kafka/pom.xml 56a3ef10a9071929776cb7211bdb8ead921deace 
>   sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java 7c459993efb811b7ee11430f42791d1083f8d9df 
>   sentry-tests/sentry-tests-solr/pom.xml c70476808688c80e1723d5e65e3b8cf6d1b64250 
>   sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java ccea82ee8ee2b976f14bc6da46a84c764b3b96f9 
>   sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 80f158a1fd626cdddeae9e2ee264f361d78bc2a7 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/5/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: Post review changes

Posted by Sergio Pena <se...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184692
-----------------------------------------------------------




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 86-90 (patched)
<https://reviews.apache.org/r/62107/#comment260881>

    This code is repeated twice. Shouldn't be good to create a private method that retrieves the full image or an empty list if one is being loaded?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 120 (patched)
<https://reviews.apache.org/r/62107/#comment260880>

    Do we need the 'seqNum <= SEQUENCE_NUMBER_UPDATE_UNINITIALIZED' condition?
    
    If an HDFS request comes in with a seqNum = 5, then we will send a new snapshot even if one is being loaded on the HMSFollower?



sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
Lines 67-69 (patched)
<https://reviews.apache.org/r/62107/#comment260882>

    Which code path we're executing here? The condition that appears on the start that checks if a snapshot is required is not executed:
    
        if (curImgNum > imgNum) {
            if (SentryServiceFactory.getInstance().isFullUpdateRunning()) {
              LOGGER.debug(
                  "A full update is being loaded. Delaying updating client with full image until its finished.");
              return Collections.emptyList();
            }
            // In case a new HMS snapshot has been processed, then return a full paths image.
            LOGGER.info("A newer full update is found with image number: {}", curImgNum);
            return Collections.singletonList(imageRetriever.retrieveFullImage());
          }
        
    curImgNum = 1
    imgNum = 1



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 299 (patched)
<https://reviews.apache.org/r/62107/#comment260885>

    Is this assertion displayed on logs as an error?
    Could this error happen at some point? HMSFollower is not configured to run by several threads, I think we're safe, aren't we?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java
Line 25 (original), 33 (patched)
<https://reviews.apache.org/r/62107/#comment260887>

    Is the 'instance' object overriden if we call create() more than once?
    
    I've seen code where the create() or getInstance() checks if instance is null before creating the object, if it is not null, then it justs returns the instance without creating it again. Maybe we could merge create() and getInstance() in one method?


- Sergio Pena


On Sept. 6, 2017, 3:52 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2017, 3:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected
> 
> Added fix to limit sending back of full snapshot when the HMSFollower is pulling a full snapshot
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/4/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: Post review changes

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184691
-----------------------------------------------------------



I added an additional unit test for a missed case

- Brian Towles


On Sept. 6, 2017, 10:52 a.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2017, 10:52 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected
> 
> Added fix to limit sending back of full snapshot when the HMSFollower is pulling a full snapshot
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/4/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.


> On Sept. 8, 2017, 1:04 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/62107/diff/6/?file=1817972#file1817972line31>
> >
> >     Someone may think that this class is thread safe, while it isn't. Is it worth making it completely thread-safe given that it is a generic utility class?

Yeah doesnt hurt.  Adding locks.


- Brian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184945
-----------------------------------------------------------


On Sept. 8, 2017, 11:29 a.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 11:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/7/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184945
-----------------------------------------------------------



The approach is fine, the comments below are mostly about documentation about thread-safety of the state bank.


sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 105 (original), 107 (patched)
<https://reviews.apache.org/r/62107/#comment261153>

    Sequence numbers do not share namespace with notification IDs, so this might confuse readers



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 281 (patched)
<https://reviews.apache.org/r/62107/#comment261156>

    As I learned these asserts do not do anything at all since they are usually never enabled, so Preconditions are better - they will be triggered in testing.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java
Lines 17 (patched)
<https://reviews.apache.org/r/62107/#comment261157>

    Please provide more documentation about these - what are these states and what is their semantics.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java
Lines 17 (patched)
<https://reviews.apache.org/r/62107/#comment261158>

    Please provide more documentation about these states and their semantics. This is public API for these states.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java
Lines 18 (patched)
<https://reviews.apache.org/r/62107/#comment261159>

    This is a publi cgeneric interface, please document what it is and how it should be used, as long as document interface getValue() method.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 27 (patched)
<https://reviews.apache.org/r/62107/#comment261160>

    Please document API and thread safety.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 28 (patched)
<https://reviews.apache.org/r/62107/#comment261164>

    make class final?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 31 (patched)
<https://reviews.apache.org/r/62107/#comment261161>

    Someone may think that this class is thread safe, while it isn't. Is it worth making it completely thread-safe given that it is a generic utility class?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 53 (patched)
<https://reviews.apache.org/r/62107/#comment261162>

    This part isn;t thread safe since it does non-atomic read/modify/write


- Alexander Kolbasov


On Sept. 7, 2017, 8:43 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2017, 8:43 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c234eafb430b58bfd1e739847e83937a5e34d878 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/6/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Na Li <li...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review185002
-----------------------------------------------------------




sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
Lines 67 (patched)
<https://reviews.apache.org/r/62107/#comment261234>

    In code DBUpdateForwarder.java line 107, you use "SEQUENCE_NUMBER_FULL_UPDATE_REQUEST " in check condition. Should you use "SEQUENCE_NUMBER_FULL_UPDATE_REQUEST " here to be consistent? the same for line 81.
    
    either all of them use "SEQUENCE_NUMBER_FULL_UPDATE_REQUEST ", or all use "SentryStore.EMPTY_NOTIFICATION_ID"



sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
Lines 113 (patched)
<https://reviews.apache.org/r/62107/#comment261235>

    keep the input "SentryStore.EMPTY_PATHS_SNAPSHOT_ID" consistent with code like above comment


- Na Li


On Sept. 8, 2017, 4:29 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 4:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/7/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Na Li <li...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184999
-----------------------------------------------------------




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
Lines 29 (patched)
<https://reviews.apache.org/r/62107/#comment261233>

    Can we just keep a single defintion for sequence_number? Otherwise, it is easy to cause confusion.
    
    Can you change those defintions to below. So it is easy for others to see the relationship
    
    EMPTY_NOTIFICATION_ID = SEQUENCE_NUMBER_UPDATE_UNINITIALIZED + 1;
    
    SEQUENCE_NUMBER_FULL_UPDATE_REQUEST = SEQUENCE_NUMBER_UPDATE_UNINITIALIZED + 1;


- Na Li


On Sept. 8, 2017, 4:29 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 4:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/7/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.


> On Sept. 8, 2017, 3:03 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java
> > Lines 20 (patched)
> > <https://reviews.apache.org/r/62107/diff/7/?file=1818513#file1818513line20>
> >
> >     Where is this class used? I cannot find anywhere you are using this class.

Looks like the useages got dropped somehow . adding back


- Brian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review185004
-----------------------------------------------------------


On Sept. 8, 2017, 2:58 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 2:58 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/8/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Na Li <li...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review185004
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java
Lines 20 (patched)
<https://reviews.apache.org/r/62107/#comment261237>

    Where is this class used? I cannot find anywhere you are using this class.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 146 (patched)
<https://reviews.apache.org/r/62107/#comment261240>

    You can get the long from states, and the call isEnabled() only once. Performance wise, should be better


- Na Li


On Sept. 8, 2017, 7:58 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 7:58 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/8/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Na Li <li...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review185014
-----------------------------------------------------------


Ship it!




Ship It!

- Na Li


On Sept. 8, 2017, 8:28 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 8:28 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/9/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Sergio Pena <se...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review185027
-----------------------------------------------------------


Ship it!




Ship It!

- Sergio Pena


On Sept. 8, 2017, 10:19 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 10:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/10/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 8, 2017, 5:19 p.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Changes
-------

Documentation clean up and return cleanup


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description
-------

Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 


Diff: https://reviews.apache.org/r/62107/diff/10/

Changes: https://reviews.apache.org/r/62107/diff/9-10/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 8, 2017, 3:28 p.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Changes
-------

Minor optimizations


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description
-------

Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 


Diff: https://reviews.apache.org/r/62107/diff/9/

Changes: https://reviews.apache.org/r/62107/diff/8-9/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 8, 2017, 2:58 p.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Changes
-------

Clean up const


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description
-------

Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 


Diff: https://reviews.apache.org/r/62107/diff/8/

Changes: https://reviews.apache.org/r/62107/diff/7-8/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.


> On Sept. 8, 2017, 1:55 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Lines 123 (patched)
> > <https://reviews.apache.org/r/62107/diff/7/?file=1818510#file1818510line128>
> >
> >     Previous log messages have INFO level. I think we should set this as INFO as well so that users know that a snapshot is requested but the client will be delayed.
> 
> Brian Towles wrote:
>     Becasue this message can occur every .5 seconds if an update is processing a flood a info level

This will potentially happen every 1/2 second while a full update is running.  That would flood the INFO level.


- Brian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184996
-----------------------------------------------------------


On Sept. 8, 2017, 11:29 a.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 11:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/7/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.


> On Sept. 8, 2017, 1:55 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Line 105 (original), 107 (patched)
> > <https://reviews.apache.org/r/62107/diff/7/?file=1818510#file1818510line112>
> >
> >     Why did you change the constant here? I'm trying to remember why we left this condition here. The old constan is -1 but the new you changed is 0.
> >     
> >     I think we were trying to keep some compatibility with an old issue we saw on the HDFS requests, but I don't remember.

Per Lina comment above

We should check if( seqNum > EMPTY_NOTIFICATION_ID && ...)

When client sends seqNum, it sends the inital value (-1) + 1, so the seqNum received by sentry server is 0. And it should get full snapshot, not delta.

Definitions are below.

// number used in authz paths and permissions to request initial syncs
  public static final long SEQUENCE_NUMBER_UPDATE_UNINITIALIZED = -1L;

public static final long EMPTY_NOTIFICATION_ID = 0L;

the server should receive 0 (EMPTY_NOTIFICATION_ID) in real operation, so the input should be 0, not -1.

In UpdateableAuthzPaths, the seqNum is initialized to -1.
seqNum = new AtomicLong(SEQUENCE_NUMBER_UPDATE_UNINITIALIZED)

In SentryUpdater, the client (HDFS) sends (-1 + 1) to Sentry Server.

=============

return sentryClient.getAllUpdatesFrom(
          authzInfo.getAuthzPermissions().getLastUpdatedSeqNum() + 1,
          authzInfo.getAuthzPaths().getLastUpdatedSeqNum() + 1,
          authzInfo.getAuthzPaths().getLastUpdatedImgNum());
=================


> On Sept. 8, 2017, 1:55 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Lines 123 (patched)
> > <https://reviews.apache.org/r/62107/diff/7/?file=1818510#file1818510line128>
> >
> >     Previous log messages have INFO level. I think we should set this as INFO as well so that users know that a snapshot is requested but the client will be delayed.

Becasue this message can occur every .5 seconds if an update is processing a flood a info level


- Brian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184996
-----------------------------------------------------------


On Sept. 8, 2017, 11:29 a.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 11:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/7/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Sergio Pena <se...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184996
-----------------------------------------------------------


Fix it, then Ship it!




The code looks good. I just have a couple of comments below. But I think we're good to go with this patch.


sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 105 (original), 107 (patched)
<https://reviews.apache.org/r/62107/#comment261228>

    Why did you change the constant here? I'm trying to remember why we left this condition here. The old constan is -1 but the new you changed is 0.
    
    I think we were trying to keep some compatibility with an old issue we saw on the HDFS requests, but I don't remember.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 123 (patched)
<https://reviews.apache.org/r/62107/#comment261229>

    Previous log messages have INFO level. I think we should set this as INFO as well so that users know that a snapshot is requested but the client will be delayed.


- Sergio Pena


On Sept. 8, 2017, 4:29 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 4:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/7/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 8, 2017, 11:29 a.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Changes
-------

Added documentation and threadsafe changes


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description
-------

Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java 0ff717d68f146459862806d6d813f0131949777f 
  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java bb85c1381b6cd5126148596bb1637f9b3a9de5fd 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 830d00eb467b47ef2abcd9f55ec0133a9d395518 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 4d83ad5262e6c1a5bae3dc290a00bf9cc1a91faa 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 


Diff: https://reviews.apache.org/r/62107/diff/7/

Changes: https://reviews.apache.org/r/62107/diff/6-7/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 7, 2017, 3:43 p.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Changes
-------

Converted to a Singleton for state notification


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description (updated)
-------

Check the state if the SentryService to see if its doing a full update and delay the NN requests for a full update while thats going on.


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c234eafb430b58bfd1e739847e83937a5e34d878 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java PRE-CREATION 


Diff: https://reviews.apache.org/r/62107/diff/6/

Changes: https://reviews.apache.org/r/62107/diff/5-6/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184796
-----------------------------------------------------------



This is becoming quite big fix. I think it is better to split it into two independent parts:

1. Refactoring changes affecting the service factory
2. Actual changes for synchronization between components

That said, I think that it is possible to have much simpler fix for the original problem that doesn't require any of these changes. The idea is to create a helper singleton class where parties can register (by name) and say - hey, I am busy, hey, I am available - using a simple map from name to atomic value. This way we do not require all these plumbing changes (which may be useful for other purposes but that's another story).

- Alexander Kolbasov


On Sept. 6, 2017, 11:33 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2017, 11:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Converted SentryService to a singleton and have HMSFollower setting a flag when a full update is running.  Set the DBUpdateForwarder to only send back full updates when then full update is not running.  Changed tests and added a Helper to support the new singleton.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java 4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java 8959ad8ec77a1268a755faf451d99a81d87cb889 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java 3b3b30e0bdf65e774d854e8252339960afcdd306 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java fe4164d4980738e8df6e93ee5f78e82e1d874ae1 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 32e67b9f8efbbec12e93794f0ab00d5b8ed555b1 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 6895720f6e6d5f12ce468d7368e0d0ee9a0fae88 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java b416ef81db21dfcf0157b2947c53721d5bcdf560 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 4cfb1f74cd90dbdb7c1aa5be07729e68353dd501 
>   sentry-tests/sentry-tests-kafka/pom.xml 56a3ef10a9071929776cb7211bdb8ead921deace 
>   sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java 7c459993efb811b7ee11430f42791d1083f8d9df 
>   sentry-tests/sentry-tests-solr/pom.xml c70476808688c80e1723d5e65e3b8cf6d1b64250 
>   sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java ccea82ee8ee2b976f14bc6da46a84c764b3b96f9 
>   sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 80f158a1fd626cdddeae9e2ee264f361d78bc2a7 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/5/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/#review184760
-----------------------------------------------------------



Of note, there is no reason to be concerned about concurrency with the changes to the SentryService.  It is now a singleton and is only configured and has one thread with one instance running at a time.

- Brian Towles


On Sept. 6, 2017, 6:33 p.m., Brian Towles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2017, 6:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
>     https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Converted SentryService to a singleton and have HMSFollower setting a flag when a full update is running.  Set the DBUpdateForwarder to only send back full updates when then full update is not running.  Changed tests and added a Helper to support the new singleton.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java 4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java 8959ad8ec77a1268a755faf451d99a81d87cb889 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java 3b3b30e0bdf65e774d854e8252339960afcdd306 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java fe4164d4980738e8df6e93ee5f78e82e1d874ae1 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 32e67b9f8efbbec12e93794f0ab00d5b8ed555b1 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 6895720f6e6d5f12ce468d7368e0d0ee9a0fae88 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java b416ef81db21dfcf0157b2947c53721d5bcdf560 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 4cfb1f74cd90dbdb7c1aa5be07729e68353dd501 
>   sentry-tests/sentry-tests-kafka/pom.xml 56a3ef10a9071929776cb7211bdb8ead921deace 
>   sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java 7c459993efb811b7ee11430f42791d1083f8d9df 
>   sentry-tests/sentry-tests-solr/pom.xml c70476808688c80e1723d5e65e3b8cf6d1b64250 
>   sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java ccea82ee8ee2b976f14bc6da46a84c764b3b96f9 
>   sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 80f158a1fd626cdddeae9e2ee264f361d78bc2a7 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/5/
> 
> 
> Testing
> -------
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 6, 2017, 6:33 p.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Summary (updated)
-----------------

SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description (updated)
-------

Converted SentryService to a singleton and have HMSFollower setting a flag when a full update is running.  Set the DBUpdateForwarder to only send back full updates when then full update is not running.  Changed tests and added a Helper to support the new singleton.


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java 4652dc9e0a35a20780418d8e388595198a8f9ccd 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java 8959ad8ec77a1268a755faf451d99a81d87cb889 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java 3b3b30e0bdf65e774d854e8252339960afcdd306 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java fe4164d4980738e8df6e93ee5f78e82e1d874ae1 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 32e67b9f8efbbec12e93794f0ab00d5b8ed555b1 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 6895720f6e6d5f12ce468d7368e0d0ee9a0fae88 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java b416ef81db21dfcf0157b2947c53721d5bcdf560 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 4cfb1f74cd90dbdb7c1aa5be07729e68353dd501 
  sentry-tests/sentry-tests-kafka/pom.xml 56a3ef10a9071929776cb7211bdb8ead921deace 
  sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java 7c459993efb811b7ee11430f42791d1083f8d9df 
  sentry-tests/sentry-tests-solr/pom.xml c70476808688c80e1723d5e65e3b8cf6d1b64250 
  sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java ccea82ee8ee2b976f14bc6da46a84c764b3b96f9 
  sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 80f158a1fd626cdddeae9e2ee264f361d78bc2a7 


Diff: https://reviews.apache.org/r/62107/diff/5/

Changes: https://reviews.apache.org/r/62107/diff/4-5/


Testing
-------

Unit Tests


Thanks,

Brian Towles

Re: Review Request 62107: Post review changes

Posted by Brian Towles <bt...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62107/
-----------------------------------------------------------

(Updated Sept. 6, 2017, 10:52 a.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.


Bugs: SENTRY-1918
    https://issues.apache.org/jira/browse/SENTRY-1918


Repository: sentry


Description (updated)
-------

SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

Added fix to limit sending back of full snapshot when the HMSFollower is pulling a full snapshot


Diffs
-----

  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 5d744214e14d6c48194b3a0bf84d6e10070b020a 
  sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java 8fbc10048003ab4b8a38676e241ae0fd27d2392c 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java d4feb380fa0f3bd5f237609a107295a2d23eae3b 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java 1685702c6dbc9715c8885a29a80bc68509550f0b 


Diff: https://reviews.apache.org/r/62107/diff/4/


Testing
-------

Unit Tests


Thanks,

Brian Towles