You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sun Dapeng <da...@intel.com> on 2014/09/24 04:09:23 UTC
Review Request 25980: SENTRY-464 Add HASentryPolicyServiceClientImpl
for high availability and Sentry service register
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/
-----------------------------------------------------------
Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya Tirukkovalur.
Bugs: SENTRY-464
https://issues.apache.org/jira/browse/SENTRY-464
Repository: sentry
Description
-------
* Add service register in **SentryPolicyStoreProcessor**
* Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
* Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
````java
private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
while (true) {
try {
return sentryOption.doOperation();
} catch (SentryUserException e) {
throw e;
} catch (Exception e) {
LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
+ ": Error in connect current service, will retry other service.", e);
try {
renewSentryClient();
} catch (IOException e1) {
throw new SentryUserException(e1.getMessage(),e1.getCause());
}
}
}
}
````
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6843e80
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/AbstractTestWithHADbProvider.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestPrivilegeWithHAGrantOption.java PRE-CREATION
Diff: https://reviews.apache.org/r/25980/diff/
Testing
-------
The addition UnitTest is used for test client reconnect, other UnitTest passed in local
Thanks,
Sun Dapeng
Re: Review Request 25980: SENTRY-464 Sentry service register and using
InvocationHandler for SentryPolicyServiceClientFactory high availability
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/
-----------------------------------------------------------
(Updated 十一月 18, 2014, 3:50 p.m.)
Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
According Prasad's comment, will handle the failure seamlessly.
Bugs: SENTRY-464
https://issues.apache.org/jira/browse/SENTRY-464
Repository: sentry
Description
-------
* Add service register in **SentryPolicyStoreProcessor**
* Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
* Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
````java
private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
while (true) {
try {
return sentryOption.doOperation();
} catch (SentryUserException e) {
throw e;
} catch (Exception e) {
LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
+ ": Error in connect current service, will retry other service.", e);
try {
renewSentryClient();
} catch (IOException e1) {
throw new SentryUserException(e1.getMessage(),e1.getCause());
}
}
}
}
````
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 4774b90
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java b19b79c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
Diff: https://reviews.apache.org/r/25980/diff/
Testing
-------
The addition UnitTest is used for test client reconnect, other UnitTest passed in local
Thanks,
Dapeng Sun
Re: Review Request 25980: SENTRY-464 Sentry service register and using
InvocationHandler for SentryPolicyServiceClientFactory high availability
Posted by Dapeng Sun <da...@intel.com>.
> On 十一月 18, 2014, 11:15 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java, line 70
> > <https://reviews.apache.org/r/25980/diff/2/?file=733885#file733885line70>
> >
> > should it be INFO ? After all we are handling the failure seamlessly, so this does'n have to be a warning ...
Good suggestion, thank you for your review.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/#review61864
-----------------------------------------------------------
On 十月 27, 2014, 3:15 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25980/
> -----------------------------------------------------------
>
> (Updated 十月 27, 2014, 3:15 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-464
> https://issues.apache.org/jira/browse/SENTRY-464
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add service register in **SentryPolicyStoreProcessor**
> * Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
> * Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
> ````java
> private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
> while (true) {
> try {
> return sentryOption.doOperation();
> } catch (SentryUserException e) {
> throw e;
> } catch (Exception e) {
> LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
> + ": Error in connect current service, will retry other service.", e);
> try {
> renewSentryClient();
> } catch (IOException e1) {
> throw new SentryUserException(e1.getMessage(),e1.getCause());
> }
> }
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25980/diff/
>
>
> Testing
> -------
>
> The addition UnitTest is used for test client reconnect, other UnitTest passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25980: SENTRY-464 Sentry service register and using
InvocationHandler for SentryPolicyServiceClientFactory high availability
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/#review61864
-----------------------------------------------------------
Ship it!
Looks fine to me. Just a minor comment below. Please feel free to address that in the attached patch on the ticket.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java
<https://reviews.apache.org/r/25980/#comment103774>
should it be INFO ? After all we are handling the failure seamlessly, so this does'n have to be a warning ...
- Prasad Mujumdar
On Oct. 27, 2014, 7:15 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25980/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2014, 7:15 a.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-464
> https://issues.apache.org/jira/browse/SENTRY-464
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add service register in **SentryPolicyStoreProcessor**
> * Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
> * Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
> ````java
> private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
> while (true) {
> try {
> return sentryOption.doOperation();
> } catch (SentryUserException e) {
> throw e;
> } catch (Exception e) {
> LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
> + ": Error in connect current service, will retry other service.", e);
> try {
> renewSentryClient();
> } catch (IOException e1) {
> throw new SentryUserException(e1.getMessage(),e1.getCause());
> }
> }
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25980/diff/
>
>
> Testing
> -------
>
> The addition UnitTest is used for test client reconnect, other UnitTest passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25980: SENTRY-464 Sentry service register and using
InvocationHandler for SentryPolicyServiceClientFactory high availability
Posted by Sun Dapeng <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/
-----------------------------------------------------------
(Updated 十月 27, 2014, 3:15 p.m.)
Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Summary (updated)
-----------------
SENTRY-464 Sentry service register and using InvocationHandler for SentryPolicyServiceClientFactory high availability
Bugs: SENTRY-464
https://issues.apache.org/jira/browse/SENTRY-464
Repository: sentry
Description
-------
* Add service register in **SentryPolicyStoreProcessor**
* Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
* Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
````java
private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
while (true) {
try {
return sentryOption.doOperation();
} catch (SentryUserException e) {
throw e;
} catch (Exception e) {
LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
+ ": Error in connect current service, will retry other service.", e);
try {
renewSentryClient();
} catch (IOException e1) {
throw new SentryUserException(e1.getMessage(),e1.getCause());
}
}
}
}
````
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
Diff: https://reviews.apache.org/r/25980/diff/
Testing
-------
The addition UnitTest is used for test client reconnect, other UnitTest passed in local
Thanks,
Sun Dapeng
Re: Review Request 25980: SENTRY-464 Add
HASentryPolicyServiceClientImpl for
high availability and Sentry service register
Posted by Sun Dapeng <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/
-----------------------------------------------------------
(Updated 十月 27, 2014, 2:51 p.m.)
Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Many thanks for Prasad's review, I updated the patch
Bugs: SENTRY-464
https://issues.apache.org/jira/browse/SENTRY-464
Repository: sentry
Description
-------
* Add service register in **SentryPolicyStoreProcessor**
* Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
* Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
````java
private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
while (true) {
try {
return sentryOption.doOperation();
} catch (SentryUserException e) {
throw e;
} catch (Exception e) {
LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
+ ": Error in connect current service, will retry other service.", e);
try {
renewSentryClient();
} catch (IOException e1) {
throw new SentryUserException(e1.getMessage(),e1.getCause());
}
}
}
}
````
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
Diff: https://reviews.apache.org/r/25980/diff/
Testing
-------
The addition UnitTest is used for test client reconnect, other UnitTest passed in local
Thanks,
Sun Dapeng
Re: Review Request 25980: SENTRY-464 Add
HASentryPolicyServiceClientImpl for
high availability and Sentry service register
Posted by Sun Dapeng <da...@intel.com>.
> On 十月 21, 2014, 8:24 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java, line 64
> > <https://reviews.apache.org/r/25980/diff/1/?file=704029#file704029line64>
> >
> > Do we need to check for a timeout here or the retry policy (in HAContext ?) will take care of it ?
Yes, HAContext will handle the retry policy and throw IOException, which may cause by connection timeout or Zookeeper service down.
> On 十月 21, 2014, 8:24 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java, line 90
> > <https://reviews.apache.org/r/25980/diff/1/?file=704029#file704029line90>
> >
> > In secure connection the client would need the kerberos principal of the server. How do we handle that case ?
Hi Prasad, the patch had the following code in renewSentryClient **conf.set(ServiceConstants.ClientConfig.SERVER_RPC_ADDRESS, serverAddress.getHostName());**, and currently we get the server principal using **serverPrincipal = SecurityUtil.getServerPrincipal(serverPrincipal, serverAddress.getAddress());** the security related Tests are added in SENTRY-459, do you think it's okay?
> On 十月 21, 2014, 8:24 a.m., Prasad Mujumdar wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/AbstractTestWithHADbProvider.java, line 1
> > <https://reviews.apache.org/r/25980/diff/1/?file=704033#file704033line1>
> >
> > Can we move the HA specific changes in the AbstractTestWithDbProvider ? That would make it simpler to run the existing tests on real cluster with HA
Good suggestion, I will make it.
> On 十月 21, 2014, 8:24 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java, line 54
> > <https://reviews.apache.org/r/25980/diff/1/?file=704029#file704029line54>
> >
> > The current approach requires an wrapper for every Sentry client method. When we add new methods, it will require to add a wrapper in here as well.
> > Have you considered using InvocationHandler for the retry wrapper ? It would make adding new methods in the client interface simpler.
> > If there's no specific reason for not using InvocationHandler, then we might want to log a followup ticket to refactor this module.
Okay, I will refact the code with InvocationHandler
- Sun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/#review57483
-----------------------------------------------------------
On 十月 9, 2014, 1:48 p.m., Sun Dapeng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25980/
> -----------------------------------------------------------
>
> (Updated 十月 9, 2014, 1:48 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-464
> https://issues.apache.org/jira/browse/SENTRY-464
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add service register in **SentryPolicyStoreProcessor**
> * Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
> * Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
> ````java
> private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
> while (true) {
> try {
> return sentryOption.doOperation();
> } catch (SentryUserException e) {
> throw e;
> } catch (Exception e) {
> LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
> + ": Error in connect current service, will retry other service.", e);
> try {
> renewSentryClient();
> } catch (IOException e1) {
> throw new SentryUserException(e1.getMessage(),e1.getCause());
> }
> }
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6843e80
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/AbstractTestWithHADbProvider.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestPrivilegeWithHAGrantOption.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25980/diff/
>
>
> Testing
> -------
>
> The addition UnitTest is used for test client reconnect, other UnitTest passed in local
>
>
> Thanks,
>
> Sun Dapeng
>
>
Re: Review Request 25980: SENTRY-464 Sentry service register and using
InvocationHandler for SentryPolicyServiceClientFactory high availability
Posted by Dapeng Sun <da...@intel.com>.
> On 十月 21, 2014, 8:24 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java, line 90
> > <https://reviews.apache.org/r/25980/diff/1/?file=704029#file704029line90>
> >
> > In secure connection the client would need the kerberos principal of the server. How do we handle that case ?
>
> Dapeng Sun wrote:
> Hi Prasad, the patch had the following code in renewSentryClient **conf.set(ServiceConstants.ClientConfig.SERVER_RPC_ADDRESS, serverAddress.getHostName());**, and currently we get the server principal using **serverPrincipal = SecurityUtil.getServerPrincipal(serverPrincipal, serverAddress.getAddress());** the security related Tests are added in SENTRY-459, do you think it's okay?
>
> Prasad Mujumdar wrote:
> I think the issue is related to adding the correct hostname in the sentry server principal. We can discuss that in SENTRY-459 review.
It's okay. Thanks for your review, if you have other comments, feel free to let me know.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/#review57483
-----------------------------------------------------------
On 十月 27, 2014, 3:15 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25980/
> -----------------------------------------------------------
>
> (Updated 十月 27, 2014, 3:15 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-464
> https://issues.apache.org/jira/browse/SENTRY-464
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add service register in **SentryPolicyStoreProcessor**
> * Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
> * Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
> ````java
> private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
> while (true) {
> try {
> return sentryOption.doOperation();
> } catch (SentryUserException e) {
> throw e;
> } catch (Exception e) {
> LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
> + ": Error in connect current service, will retry other service.", e);
> try {
> renewSentryClient();
> } catch (IOException e1) {
> throw new SentryUserException(e1.getMessage(),e1.getCause());
> }
> }
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25980/diff/
>
>
> Testing
> -------
>
> The addition UnitTest is used for test client reconnect, other UnitTest passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25980: SENTRY-464 Sentry service register and using
InvocationHandler for SentryPolicyServiceClientFactory high availability
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On Oct. 21, 2014, 12:24 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java, line 90
> > <https://reviews.apache.org/r/25980/diff/1/?file=704029#file704029line90>
> >
> > In secure connection the client would need the kerberos principal of the server. How do we handle that case ?
>
> Dapeng Sun wrote:
> Hi Prasad, the patch had the following code in renewSentryClient **conf.set(ServiceConstants.ClientConfig.SERVER_RPC_ADDRESS, serverAddress.getHostName());**, and currently we get the server principal using **serverPrincipal = SecurityUtil.getServerPrincipal(serverPrincipal, serverAddress.getAddress());** the security related Tests are added in SENTRY-459, do you think it's okay?
I think the issue is related to adding the correct hostname in the sentry server principal. We can discuss that in SENTRY-459 review.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/#review57483
-----------------------------------------------------------
On Oct. 27, 2014, 7:15 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25980/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2014, 7:15 a.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-464
> https://issues.apache.org/jira/browse/SENTRY-464
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add service register in **SentryPolicyStoreProcessor**
> * Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
> * Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
> ````java
> private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
> while (true) {
> try {
> return sentryOption.doOperation();
> } catch (SentryUserException e) {
> throw e;
> } catch (Exception e) {
> LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
> + ": Error in connect current service, will retry other service.", e);
> try {
> renewSentryClient();
> } catch (IOException e1) {
> throw new SentryUserException(e1.getMessage(),e1.getCause());
> }
> }
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 47e01a7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25980/diff/
>
>
> Testing
> -------
>
> The addition UnitTest is used for test client reconnect, other UnitTest passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25980: SENTRY-464 Add
HASentryPolicyServiceClientImpl for
high availability and Sentry service register
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/#review57483
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java
<https://reviews.apache.org/r/25980/#comment98204>
The current approach requires an wrapper for every Sentry client method. When we add new methods, it will require to add a wrapper in here as well.
Have you considered using InvocationHandler for the retry wrapper ? It would make adding new methods in the client interface simpler.
If there's no specific reason for not using InvocationHandler, then we might want to log a followup ticket to refactor this module.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java
<https://reviews.apache.org/r/25980/#comment98206>
Do we need to check for a timeout here or the retry policy (in HAContext ?) will take care of it ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java
<https://reviews.apache.org/r/25980/#comment98198>
In secure connection the client would need the kerberos principal of the server. How do we handle that case ?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/AbstractTestWithHADbProvider.java
<https://reviews.apache.org/r/25980/#comment98209>
Can we move the HA specific changes in the AbstractTestWithDbProvider ? That would make it simpler to run the existing tests on real cluster with HA
- Prasad Mujumdar
On Oct. 9, 2014, 5:48 a.m., Sun Dapeng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25980/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 5:48 a.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-464
> https://issues.apache.org/jira/browse/SENTRY-464
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Add service register in **SentryPolicyStoreProcessor**
> * Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
> * Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
> ````java
> private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
> while (true) {
> try {
> return sentryOption.doOperation();
> } catch (SentryUserException e) {
> throw e;
> } catch (Exception e) {
> LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
> + ": Error in connect current service, will retry other service.", e);
> try {
> renewSentryClient();
> } catch (IOException e1) {
> throw new SentryUserException(e1.getMessage(),e1.getCause());
> }
> }
> }
> }
> ````
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6843e80
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/AbstractTestWithHADbProvider.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestPrivilegeWithHAGrantOption.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/25980/diff/
>
>
> Testing
> -------
>
> The addition UnitTest is used for test client reconnect, other UnitTest passed in local
>
>
> Thanks,
>
> Sun Dapeng
>
>
Re: Review Request 25980: SENTRY-464 Add
HASentryPolicyServiceClientImpl for
high availability and Sentry service register
Posted by Sun Dapeng <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25980/
-----------------------------------------------------------
(Updated 十月 9, 2014, 1:48 p.m.)
Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Add Lenni.
Bugs: SENTRY-464
https://issues.apache.org/jira/browse/SENTRY-464
Repository: sentry
Description
-------
* Add service register in **SentryPolicyStoreProcessor**
* Add **HASentryPolicyServiceClientImpl** as a HA implementation for SentryPolicyServiceClient, it can select active node which registered in Zookeeper
* Add **doOperationAndRetry** , use **SentryPolicyServiceClientDefaultImpl** as a field, this make all HA method can reuse the same logic for retry.
````java
private <T> T doOperationAndRetry(SentryOperation<T> sentryOption) throws SentryUserException {
while (true) {
try {
return sentryOption.doOperation();
} catch (SentryUserException e) {
throw e;
} catch (Exception e) {
LOGGER.warn(THRIFT_EXCEPTION_MESSAGE
+ ": Error in connect current service, will retry other service.", e);
try {
renewSentryClient();
} catch (IOException e1) {
throw new SentryUserException(e1.getMessage(),e1.getCause());
}
}
}
}
````
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/HASentryPolicyServiceClientImpl.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java e3cdfc2
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6843e80
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 11545a5
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/AbstractTestWithHADbProvider.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestPrivilegeWithHAGrantOption.java PRE-CREATION
Diff: https://reviews.apache.org/r/25980/diff/
Testing
-------
The addition UnitTest is used for test client reconnect, other UnitTest passed in local
Thanks,
Sun Dapeng