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

Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/06/05 19:25:51 UTC

Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

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

Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


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


Repository: sentry


Description
-------

Root case: 
1. Client was expecting TTransportException exception when there is failure while opening TSaslClientTransport but it was throwing UndeclaredThrowableException with cause asTTransportException.
2. On failure with first server sentry client tried to connect to the second sentry server with wrong principle.


Diffs
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 74aced2 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab 


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


Testing
-------

Made sure that all the existing units passed. To test the exact fix we need test framework to test with multiple sentry servers. I have an outstanding code review with that changes. Once that changes are comitted we can add tests for multiple sentry servres.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

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


Fix it, then Ship it!





sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Line 277 (original), 279 (patched)
<https://reviews.apache.org/r/59810/#comment250490>

    Just do the variable declaration right here itself.
    String[] serverPrincipalParts = SaslRpcServer.splitKerberosName(serverPrincipal);


- Vamsee Yarlagadda


On June 5, 2017, 7:25 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59810/
> -----------------------------------------------------------
> 
> (Updated June 5, 2017, 7:25 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1791
>     https://issues.apache.org/jira/browse/SENTRY-1791
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Root case: 
> 1. Client was expecting TTransportException exception when there is failure while opening TSaslClientTransport but it was throwing UndeclaredThrowableException with cause asTTransportException.
> 2. On failure with first server sentry client tried to connect to the second sentry server with wrong principle.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 74aced2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab 
> 
> 
> Diff: https://reviews.apache.org/r/59810/diff/1/
> 
> 
> Testing
> -------
> 
> Made sure that all the existing units passed. To test the exact fix we need test framework to test with multiple sentry servers. I have an outstanding code review with that changes. Once that changes are comitted we can add tests for multiple sentry servres.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

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


Ship it!




Ship It!

- Vamsee Yarlagadda


On June 7, 2017, 12:58 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59810/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 12:58 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1791
>     https://issues.apache.org/jira/browse/SENTRY-1791
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Root case: 
> 1. Client was expecting TTransportException exception when there is failure while opening TSaslClientTransport but it was throwing UndeclaredThrowableException with cause asTTransportException.
> 2. On failure with first server sentry client tried to connect to the second sentry server with wrong principle.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java 34a594e 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 74aced2 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java 798bbef 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java e23d13b 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java d1a4d99 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab 
> 
> 
> Diff: https://reviews.apache.org/r/59810/diff/2/
> 
> 
> Testing
> -------
> 
> Made sure that all the existing units passed. To test the exact fix we need test framework to test with multiple sentry servers. I have an outstanding code review with that changes. Once that changes are comitted we can add tests for multiple sentry servres.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

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

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


Fix it, then Ship it!





sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
Lines 152 (patched)
<https://reviews.apache.org/r/59810/#comment251577>

    This is the last statement, no need to continue



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Lines 102 (patched)
<https://reviews.apache.org/r/59810/#comment251578>

    Use {} formatting for the log


- Alexander Kolbasov


On June 13, 2017, 11:41 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59810/
> -----------------------------------------------------------
> 
> (Updated June 13, 2017, 11:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1791
>     https://issues.apache.org/jira/browse/SENTRY-1791
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Root case: 
> 1. Client was expecting TTransportException exception when there is failure while opening TSaslClientTransport but it was throwing UndeclaredThrowableException with cause asTTransportException.
> 2. On failure with first server sentry client tried to connect to the second sentry server with wrong principle.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java 62d0d2c 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java d299113 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/TransportFactory.java e115cbb 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab 
> 
> 
> Diff: https://reviews.apache.org/r/59810/diff/3/
> 
> 
> Testing
> -------
> 
> Made sure that all the existing units passed. To test the exact fix we need test framework to test with multiple sentry servers. I have an outstanding code review with that changes. Once that changes are comitted we can add tests for multiple sentry servres.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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

(Updated June 13, 2017, 11:41 p.m.)

Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.

Changes
-------

Based the patch with the recent commit made to have connection pool towards sentry server.

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

Repository: sentry

Description
-------

Root case:
1. Client was expecting TTransportException exception when there is failure while opening TSaslClientTransport but it was throwing UndeclaredThrowableException with cause asTTransportException.
2. On failure with first server sentry client tried to connect to the second sentry server with wrong principle.

Diffs (updated)
-----

sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java 62d0d2c
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java d299113
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/TransportFactory.java e115cbb
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab

Diff: https://reviews.apache.org/r/59810/diff/3/

Changes: https://reviews.apache.org/r/59810/diff/2-3/

Testing
-------

Made sure that all the existing units passed. To test the exact fix we need test framework to test with multiple sentry servers. I have an outstanding code review with that changes. Once that changes are comitted we can add tests for multiple sentry servres.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

(Updated June 7, 2017, 12:58 a.m.)

Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.

Changes
-------

Addressed review comments.

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

Repository: sentry

Description
-------

Diffs (updated)
-----

sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java 34a594e
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 74aced2
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java 798bbef
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java e23d13b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java d1a4d99
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab

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

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

Testing
-------

Thanks,

kalyan kumar kalvagadda

Re: Review Request 59810: SENTRY-1791 Sentry Clients failover not working with kerberos enabled

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

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



Should we do the exception juggling here when we create connection or in the retry client handler where we decide when to retry?


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Line 228 (original), 225 (patched)
<https://reviews.apache.org/r/59810/#comment250513>

    What's wrong with the original message?
    
    Since you are changing this line, let's use logger formatting:
    
    LOGGER.error("Failed connection to Sentry server {}", addr);



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Lines 245 (patched)
<https://reviews.apache.org/r/59810/#comment250514>

    Please add comment to this class explaining the non-trivial exception handling.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Lines 250 (patched)
<https://reviews.apache.org/r/59810/#comment250515>

    We don't need getMessge, can just use e as an argument



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Lines 269 (patched)
<https://reviews.apache.org/r/59810/#comment250516>

    Move the declaration to the place where it is actually used.


- Alexander Kolbasov


On June 5, 2017, 7:25 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59810/
> -----------------------------------------------------------
> 
> (Updated June 5, 2017, 7:25 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1791
>     https://issues.apache.org/jira/browse/SENTRY-1791
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Root case: 
> 1. Client was expecting TTransportException exception when there is failure while opening TSaslClientTransport but it was throwing UndeclaredThrowableException with cause asTTransportException.
> 2. On failure with first server sentry client tried to connect to the second sentry server with wrong principle.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 74aced2 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab 
> 
> 
> Diff: https://reviews.apache.org/r/59810/diff/1/
> 
> 
> Testing
> -------
> 
> Made sure that all the existing units passed. To test the exact fix we need test framework to test with multiple sentry servers. I have an outstanding code review with that changes. Once that changes are comitted we can add tests for multiple sentry servres.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>