You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/07/11 15:46:45 UTC

Review Request 60775: SENTRY-1630 out of sequence error in HMSFollower

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

Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.


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


Repository: sentry


Description
-------

SENTRY-1630 out of sequence error in HMSFollower


Diffs
-----

  sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef 
  sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java 389e9b8457aa03b44e2ad94c405d3dec7e422de8 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621 


Diff: https://reviews.apache.org/r/60775/diff/1/


Testing
-------


Thanks,

Alexander Kolbasov

Re: Review Request 60775: SENTRY-1630 out of sequence error in HMSFollower

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

> On July 11, 2017, 5:39 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 79 (original), 72 (patched)
> > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line80>
> >
> >     do we still need this variable? There is hiveConnectionFactory to create the client.

Yes, we still need this - it is the actual open client.


> On July 11, 2017, 5:39 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 106 (original), 99 (patched)
> > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line109>
> >
> >     if client.invalidate() is called, should you return false here? Then this function should check if client is invalidated.

When invalidate() is called, we call `closeHMSConnection` which sets it to false.


> On July 11, 2017, 5:39 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 122 (original), 120 (patched)
> > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line130>
> >
> >     To be safe, can you check if client is valid? if not, create a new client.

There is no method to check validity of the client. Users are not supposed to use invalidated or closed clients.


> On July 11, 2017, 5:39 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 282 (patched)
> > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line373>
> >
> >     should you set client = null after close it? Otherwise, previous exception could cause client to close, and HMSFollower client does not work any more.

It is set to null in finally clause


> On July 11, 2017, 5:39 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java
> > Lines 25 (patched)
> > <https://reviews.apache.org/r/60775/diff/1/?file=1774234#file1774234line25>
> >
> >     Maybe in another Jira, can we have a connection pool, so we can reuse the connection if it is valid once it is used? That will improve performance and make getting full snapshot faster.

There is another JIRA open for Hive. If they don't do it may be we'll do it in Sentry, but I'd prefer Hive team to provide it.


- Alexander


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


On July 11, 2017, 3:46 p.m., Alexander Kolbasov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60775/
> -----------------------------------------------------------
> 
> (Updated July 11, 2017, 3:46 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1630
>     https://issues.apache.org/jira/browse/SENTRY-1630
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1630 out of sequence error in HMSFollower
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef 
>   sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java 389e9b8457aa03b44e2ad94c405d3dec7e422de8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621 
> 
> 
> Diff: https://reviews.apache.org/r/60775/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 60775: SENTRY-1630 out of sequence error in HMSFollower

Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60775/#review180207
-----------------------------------------------------------




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
Lines 281 (patched)
<https://reviews.apache.org/r/60775/#comment255253>

    Should "Fetching" be "Fetched"?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
Line 365 (original), 393 (patched)
<https://reviews.apache.org/r/60775/#comment255256>

    can you change input "client" to "clientFactory"? It is easier to read the code



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java
Lines 33 (patched)
<https://reviews.apache.org/r/60775/#comment255258>

    Do you want to check if client is not null? You call client.close() without checking if it is null in invalidate()



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java
Lines 42 (patched)
<https://reviews.apache.org/r/60775/#comment255259>

    To be consistent with close(), can you check if valid is true before calling client.close()? 
    
    Do you know if we can call client.close() multiple times?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 79 (original), 72 (patched)
<https://reviews.apache.org/r/60775/#comment255260>

    do we still need this variable? There is hiveConnectionFactory to create the client.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 106 (original), 99 (patched)
<https://reviews.apache.org/r/60775/#comment255261>

    if client.invalidate() is called, should you return false here? Then this function should check if client is invalidated.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 122 (original), 120 (patched)
<https://reviews.apache.org/r/60775/#comment255268>

    To be safe, can you check if client is valid? if not, create a new client.



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

    should you set client = null after close it? Otherwise, previous exception could cause client to close, and HMSFollower client does not work any more.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java
Lines 25 (patched)
<https://reviews.apache.org/r/60775/#comment255277>

    Maybe in another Jira, can we have a connection pool, so we can reuse the connection if it is valid once it is used? That will improve performance and make getting full snapshot faster.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
Lines 57 (patched)
<https://reviews.apache.org/r/60775/#comment255274>

    Can you change "conf" to "sentryConf"? It is more clear


- Na Li


On July 11, 2017, 3:46 p.m., Alexander Kolbasov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60775/
> -----------------------------------------------------------
> 
> (Updated July 11, 2017, 3:46 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1630
>     https://issues.apache.org/jira/browse/SENTRY-1630
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1630 out of sequence error in HMSFollower
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef 
>   sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java 389e9b8457aa03b44e2ad94c405d3dec7e422de8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621 
> 
> 
> Diff: https://reviews.apache.org/r/60775/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 60775: SENTRY-1630 out of sequence error in HMSFollower

Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60775/#review180231
-----------------------------------------------------------


Ship it!




Ship It!

- Na Li


On July 11, 2017, 6:21 p.m., Alexander Kolbasov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60775/
> -----------------------------------------------------------
> 
> (Updated July 11, 2017, 6:21 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1630
>     https://issues.apache.org/jira/browse/SENTRY-1630
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1630 out of sequence error in HMSFollower
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef 
>   sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java 389e9b8457aa03b44e2ad94c405d3dec7e422de8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621 
> 
> 
> Diff: https://reviews.apache.org/r/60775/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 60775: SENTRY-1630 out of sequence error in HMSFollower

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60775/
-----------------------------------------------------------

(Updated July 11, 2017, 6:21 p.m.)


Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.


Changes
-------

Addressed code review change from Lina


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


Repository: sentry


Description
-------

SENTRY-1630 out of sequence error in HMSFollower


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef 
  sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java 389e9b8457aa03b44e2ad94c405d3dec7e422de8 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621 


Diff: https://reviews.apache.org/r/60775/diff/2/

Changes: https://reviews.apache.org/r/60775/diff/1-2/


Testing
-------


Thanks,

Alexander Kolbasov

Re: Review Request 60775: SENTRY-1630 out of sequence error in HMSFollower

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60775/#review180211
-----------------------------------------------------------


Ship it!




Ship It!

- Vamsee Yarlagadda


On July 11, 2017, 3:46 p.m., Alexander Kolbasov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60775/
> -----------------------------------------------------------
> 
> (Updated July 11, 2017, 3:46 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1630
>     https://issues.apache.org/jira/browse/SENTRY-1630
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SENTRY-1630 out of sequence error in HMSFollower
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef 
>   sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java 389e9b8457aa03b44e2ad94c405d3dec7e422de8 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621 
> 
> 
> Diff: https://reviews.apache.org/r/60775/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>