You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Hao Hao <ha...@cloudera.com> on 2016/07/28 18:32:55 UTC
Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/
-----------------------------------------------------------
Review request for sentry and Sravya Tirukkovalur.
Repository: sentry
Description
-------
The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
Diffs
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
Diff: https://reviews.apache.org/r/50578/diff/
Testing
-------
Thanks,
Hao Hao
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Hao Hao <ha...@cloudera.com>.
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > Thanks for picking this up Hao! Appreciate it! Do we need to update the non pool java clients as well?
Yeah, good point. Tend to deprecated non pool java clients in this case. Do you know any reason why we have to keep it?
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java, line 107
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456880#file1456880line107>
> >
> > Is this change in behavior required?
Yeah, the expected excpetion now become nested inside.
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, lines 260-268
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line260>
> >
> > Seems like endPointIdx will always be the same as curFreshestEndpointIdx?
Yeah, curFreshestEndpointIdx is working as a temp variable to set the freshestEndpointIdx value.
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, lines 273-278
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line273>
> >
> > It is not very clear what we are trying to achieve here. Seems like the order of endpoints matter as per this logic?
Here is after reach the retry max, checking for all exception received is it caused by standby or active is unreachable. The order of endpoints does not matter.
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java, line 26
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456881#file1456881line26>
> >
> > I only see tests for hostString parsing logic, shall we add tests for actual standbyException handling?
I think it is not straightforward to do it here, would be better to do in the failover e2e test?
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144007
-----------------------------------------------------------
On July 28, 2016, 9:14 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated July 28, 2016, 9:14 p.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Hao Hao <ha...@cloudera.com>.
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, lines 273-278
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line273>
> >
> > It is not very clear what we are trying to achieve here. Seems like the order of endpoints matter as per this logic?
>
> Hao Hao wrote:
> Here is after reach the retry max, checking for all exception received is it caused by standby or active is unreachable. The order of endpoints does not matter.
>
> Sravya Tirukkovalur wrote:
> I think we should log.error these exceptions, even if we throw only the last exception. As connection to each could have failed due to a different reason. And can we make the exception messages a bit more descriptive to make it more actionable. Basically, if we have unreachable exceptions, then there is some thing weird going on like either a kerberos issue or service name is wrong etc. On the other hand if there is no active, even if there are standbys, then it can mean passive is taking too long to become active. We should also come back to this once we support {ACTIVE, STANDBY, STARTING} states.
Good point, will updated.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144007
-----------------------------------------------------------
On July 29, 2016, 12:47 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 12:47 a.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java acb906fc8b792db70dc09821c0a6ad7cf7361807
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 48ee66a4132113d49dc16c419bd496dd1572b59d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java 3a38b243e79e007c0e15ee947828301136608525
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, lines 273-278
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line273>
> >
> > It is not very clear what we are trying to achieve here. Seems like the order of endpoints matter as per this logic?
>
> Hao Hao wrote:
> Here is after reach the retry max, checking for all exception received is it caused by standby or active is unreachable. The order of endpoints does not matter.
I think we should log.error these exceptions, even if we throw only the last exception. As connection to each could have failed due to a different reason. And can we make the exception messages a bit more descriptive to make it more actionable. Basically, if we have unreachable exceptions, then there is some thing weird going on like either a kerberos issue or service name is wrong etc. On the other hand if there is no active, even if there are standbys, then it can mean passive is taking too long to become active. We should also come back to this once we support {ACTIVE, STANDBY, STARTING} states.
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144007
-----------------------------------------------------------
On July 29, 2016, 12:47 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 12:47 a.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java acb906fc8b792db70dc09821c0a6ad7cf7361807
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 48ee66a4132113d49dc16c419bd496dd1572b59d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java 3a38b243e79e007c0e15ee947828301136608525
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144007
-----------------------------------------------------------
Thanks for picking this up Hao! Appreciate it! Do we need to update the non pool java clients as well?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 142)
<https://reviews.apache.org/r/50578/#comment209944>
Supportability: Shall we Log.info these end points during start up?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 147)
<https://reviews.apache.org/r/50578/#comment209946>
Nit: Can we add a comment on what are the valid hostsAndPortStrings here? Or just refer to the test case?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (lines 244 - 254)
<https://reviews.apache.org/r/50578/#comment209950>
Handling both exceptions similarly? If so, catch (SentryStandbyException|TTransportException e) {
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (lines 256 - 264)
<https://reviews.apache.org/r/50578/#comment209953>
Seems like endPointIdx will always be the same as curFreshestEndpointIdx?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (lines 269 - 274)
<https://reviews.apache.org/r/50578/#comment209955>
It is not very clear what we are trying to achieve here. Seems like the order of endpoints matter as per this logic?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java (line 107)
<https://reviews.apache.org/r/50578/#comment209958>
Is this change in behavior required?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java (line 26)
<https://reviews.apache.org/r/50578/#comment209960>
I only see tests for hostString parsing logic, shall we add tests for actual standbyException handling?
- Sravya Tirukkovalur
On July 28, 2016, 9:14 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated July 28, 2016, 9:14 p.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Li Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review149898
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 278)
<https://reviews.apache.org/r/50578/#comment217665>
endpointIdx -> i ?
also in line282
Also consider the situation when exec[i] == null
- Li Li
On Aug. 2, 2016, 9:55 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2016, 9:55 p.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Rahul Sharma <ra...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144804
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 273)
<https://reviews.apache.org/r/50578/#comment210914>
if (retryCount == connectionRetryTotal
Instead of connectionRetryTotal should it not be retryLimit?
- Rahul Sharma
On Aug. 2, 2016, 9:55 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2016, 9:55 p.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144570
-----------------------------------------------------------
Fix it, then Ship it!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 282)
<https://reviews.apache.org/r/50578/#comment210600>
Also log the actual exception here? So that we do no lose context of reason and stack trace of exception?
Nit: is in unreachable -> is unreachable?
- Sravya Tirukkovalur
On Aug. 2, 2016, 9:55 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2016, 9:55 p.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/
-----------------------------------------------------------
(Updated Aug. 2, 2016, 9:55 p.m.)
Review request for sentry and Sravya Tirukkovalur.
Repository: sentry
Description
-------
The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
Diff: https://reviews.apache.org/r/50578/diff/
Testing
-------
Thanks,
Hao Hao
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Hao Hao <ha...@cloudera.com>.
> On July 29, 2016, 5:17 p.m., Rahul Sharma wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java, line 26
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456881#file1456881line26>
> >
> > Can check out if SENTRY-1415 can be used to write a testcase for standbyException handling
Yeah I think we can do it in a follow up jira.
> On July 29, 2016, 5:17 p.m., Rahul Sharma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, lines 248-258
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line248>
> >
> > Is these the only two exceptions we expect to see here? If not ,may be catch a generic Exception and maybe later could do 'instanceof' to handle them seperately.
Yeah, I think you are looking at the previous version of code. :) Changed the logic based on sravya's suggestion.
> On July 29, 2016, 5:17 p.m., Rahul Sharma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, line 287
> > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line287>
> >
> > IS the comment right?
Will change it to be more clear.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144126
-----------------------------------------------------------
On July 29, 2016, 12:47 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 12:47 a.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java acb906fc8b792db70dc09821c0a6ad7cf7361807
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 48ee66a4132113d49dc16c419bd496dd1572b59d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java 3a38b243e79e007c0e15ee947828301136608525
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Rahul Sharma <ra...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/#review144126
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (lines 244 - 254)
<https://reviews.apache.org/r/50578/#comment210134>
Is these the only two exceptions we expect to see here? If not ,may be catch a generic Exception and maybe later could do 'instanceof' to handle them seperately.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 283)
<https://reviews.apache.org/r/50578/#comment210133>
IS the comment right?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java (line 26)
<https://reviews.apache.org/r/50578/#comment210135>
Can check out if SENTRY-1415 can be used to write a testcase for standbyException handling
- Rahul Sharma
On July 29, 2016, 12:47 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50578/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 12:47 a.m.)
>
>
> Review request for sentry and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java acb906fc8b792db70dc09821c0a6ad7cf7361807
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 48ee66a4132113d49dc16c419bd496dd1572b59d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java 3a38b243e79e007c0e15ee947828301136608525
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50578/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/
-----------------------------------------------------------
(Updated July 29, 2016, 12:47 a.m.)
Review request for sentry and Sravya Tirukkovalur.
Repository: sentry
Description
-------
The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java acb906fc8b792db70dc09821c0a6ad7cf7361807
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 48ee66a4132113d49dc16c419bd496dd1572b59d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java 3a38b243e79e007c0e15ee947828301136608525
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 51bba31bddc4404506bbe42f9f8d444b36d5056c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
Diff: https://reviews.apache.org/r/50578/diff/
Testing
-------
Thanks,
Hao Hao
Re: Review Request 50578: Sentry-1411: The sentry client should retry
RPCs if it gets a SentryStandbyException
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50578/
-----------------------------------------------------------
(Updated July 28, 2016, 9:14 p.m.)
Review request for sentry and Sravya Tirukkovalur.
Repository: sentry
Description
-------
The sentry client should retry configurable times RPCs if it gets a SentryStandbyException
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java b2df699b962def9ef2dbf60fe8f601f9aaeb03f5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 1039e6ebfde32ec8db82287458b8b4b6a23136a3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java a35bf1d4781c7424283610c7fa67ede990e35fef
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 56d774b883914dbffff438101f47fb94518c94d5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java afea78a0bea754f950cc7e3033e1b1ebfdcb35fc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java 15eab15b79f9057d173e0ea4ea64e152ade698d6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java PRE-CREATION
Diff: https://reviews.apache.org/r/50578/diff/
Testing
-------
Thanks,
Hao Hao