Review Request 50578: Sentry-1411: The sentry client should retry RPCs if it gets a SentryStandbyException

2016-07-28 Thread Hao Hao 

---
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
 56d774b883914db438101f47fb94518c94d5 
  
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

2016-07-28 Thread Hao Hao 

---
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
 56d774b883914db438101f47fb94518c94d5 
  
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

2016-07-28 Thread Sravya Tirukkovalur 

---
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)


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)


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)


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)


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)


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)


Is this change in behavior required?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 (line 26)


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
>  56d774b883914db438101f47fb94518c94d5 
>   
> 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

2016-07-28 Thread Hao Hao 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
>  56d774b883914db438101f47fb94518c94d5 
>   
> 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

2016-07-28 Thread Hao Hao 

---
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

2016-07-29 Thread Rahul Sharma 

---
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)


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)


IS the comment right?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 (line 26)


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

2016-07-29 Thread Hao Hao 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

2016-07-29 Thread Sravya Tirukkovalur 


> 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
> > 
> >
> > 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

2016-07-29 Thread Hao Hao 


> 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
> > 
> >
> > 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

2016-08-02 Thread Hao Hao 

---
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
 56d774b883914db438101f47fb94518c94d5 
  
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

2016-08-02 Thread Sravya Tirukkovalur 

---
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)


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
>  56d774b883914db438101f47fb94518c94d5 
>   
> 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

2016-08-04 Thread Rahul Sharma 

---
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)


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
>  56d774b883914db438101f47fb94518c94d5 
>   
> 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

2016-09-21 Thread Li Li 

---
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)


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
>  56d774b883914db438101f47fb94518c94d5 
>   
> 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
> 
>