Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Hao Hao]

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


Ship it!




Ship It!

- Hao Hao


On Nov. 9, 2016, 3:37 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 9, 2016, 3:37 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Nov. 9, 2016, 3:37 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 9, 2016, 3:37 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 216)


The difference between shuffle in constructor and between retries is that 
there is a chance that we will retry every time with the same address.


- Alexander Kolbasov


On Nov. 9, 2016, 3:37 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 9, 2016, 3:37 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 9, 2016, 3:37 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 9, 2016, 3:34 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 9, 2016, 3:33 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

- Li


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


On Nov. 5, 2016, 1:29 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 5, 2016, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

> On Nov. 9, 2016, 1:56 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 147
> > 
> >
> > Nit:
> > 
> > The recommended way of using preconditions is 
> > 
> > 1) Using static import
> > 2) Use something like this.conf = checkNotNull(this.conf, ...)

since this is not the only place to use preconditions as this way, I will keep 
it for now, and update all of them together later.


> On Nov. 9, 2016, 1:56 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 218
> > 
> >
> > I think it is better to shuffle in constructor rather than here. For 
> > example, you have servers A and B, A fails, after the shuffle you have 50% 
> > chance of retrying A again.

I think similar to shuffle before each full retry, shuffle in constructor 
cannot change the 50% chance of retrying A again unless we record the failed 
servers in the last full retry.


> On Nov. 9, 2016, 1:56 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 219
> > 
> >
> > I think we should remember the last endpoint we connected to and start 
> > from the next one rather than try from the beginning all the time. At least 
> > we should skip the failed one.

It is possible that the failed one is just a temporary failure e.g. reach the 
connection limitation, in that case we may skip good servers. We can create a 
followup jira for it if necessary.


> On Nov. 9, 2016, 1:56 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 159
> > 
> >
> > nit: use checkNotNull

keep it for now, and update it later with all other preconditions.


- Li


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


On Nov. 5, 2016, 1:29 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 5, 2016, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before cli

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 124)


Similar to the previous implementation of parseHostPortStrings, any 
exception during parsing will be handled by the caller.


- Li Li


On Nov. 5, 2016, 1:29 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 5, 2016, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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


Ship it!




None of my comments are show-stoppers, so if you don't have time to address 
them, please ship as is and we'll address them later.

- Alexander Kolbasov


On Nov. 5, 2016, 1:29 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 5, 2016, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 142)


No one is calling this constructor with connectNow set to true - do you 
think that it is useful to have this constructor argument?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 145)


Nit:

The recommended way of using preconditions is 

1) Using static import
2) Use something like this.conf = checkNotNull(this.conf, ...)



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 157)


nit: use checkNotNull



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 169)


Nit: it would be better to just use hostsAndPorts[i].toString



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 174)


This probbaly should be just removed since no one uses the constructor with 
connect.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 200)


Nit - keep TODO in a single line because IDEs show only the single line 
having TODO



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 207)


Please add a docstring comment above explaining that this is a no-op when 
already connected.

But I would argue that probably it should close and reconnect in such case 
instead.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 216)


I think it is better to shuffle in constructor rather than here. For 
example, you have servers A and B, A fails, after the shuffle you have 50% 
chance of retrying A again.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 217)


I think we should remember the last endpoint we connected to and start from 
the next one rather than try from the beginning all the time. At least we 
should skip the failed one.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 223)


Nit:

It may be cleaner to say "failed connection to %s: %s"



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 119)


nit:

it would be cleaner to get an array of strings and return an array of 
addresses allocated here rather than allocate a list of addresses externally.

Or we can even allocate an array of InetSocketAddress things right here and 
return it,



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 124)


How do we handle parsing error?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 82)


I think the isConnected state should be completely managed by the client.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 92)


Should we add 'continue' here? there is no point going forward.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 97)


Adding 'continue' above will get rid of this check


- Alexander Kolbasov


On Nov. 5, 2016, 1:29 a.m., Li Li wrote:
> 
> -

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Hao Hao]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 39)


Add a comment on if the client is already connect. What will happen.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 75)


Comment on what will happen after retry with rpcRetryTotal time?


- Hao Hao


On Nov. 5, 2016, 1:29 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 5, 2016, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 5, 2016, 1:29 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 4, 2016, 10:52 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Changes
---

Synchronized invokeImpl in RetryClientInvocationHandler to resolve the thread 
concurrent issues


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

> On Nov. 4, 2016, 10:10 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java,
> >  line 123
> > 
> >
> > It is possible that two threads call this concurrently, both discover a 
> > problem, one of them reconnects first and another one will close already 
> > reconnected client and try to connect it again which isn't good,

a simple solution is to make invokeImpl synchronized


- Li


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


On Nov. 4, 2016, 6:26 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 4, 2016, 6:26 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 96)


The problem is that isConnected doesn't know whether some other thread 
caused client close() or even reconnected, so this isn't a very useful check.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 123)


It is possible that two threads call this concurrently, both discover a 
problem, one of them reconnects first and another one will close already 
reconnected client and try to connect it again which isn't good,


- Alexander Kolbasov


On Nov. 4, 2016, 6:26 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 4, 2016, 6:26 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 4, 2016, 6:26 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

> On Nov. 4, 2016, 6:30 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java,
> >  line 74
> > 
> >
> > Can this code be executed concurrently? Will it handle isCOnnected 
> > logic correctly in this case?
> > 
> > I think it is better to put this logic for if(!isConnected) 
> > connectWithRetry inside client which is synchronized.

Since connectWithRetry is synchronized and client will be closed in 
client.connectWithRetry(), the code can be executed concurrently.
I will put the logic inside client class and call client.close() outside when 
got connection problem.


- Li


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


On Nov. 2, 2016, 6:08 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 2, 2016, 6:08 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 74)


will put it inside client



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 75)


agree. will update



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 80)


no, only connectWithRetry() can throw IOException.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 81)


Ooops, I intend to retry (see the comment), but forgot to change the code.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 89)


either way is fine for me. it is very easy to ignore the '!' in if clause. 
I will keep the current one for now.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 95)


we can add a debug log here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 96)


because it is originally TTransportException.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 100)


If changed to Exception, all current sentry client has to change the catch 
exception cod, e.g. impala.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 126)


actually client will be closed in cient.connectWithRetry(). But I will move 
it to the client


- Li Li


On Nov. 2, 2016, 6:08 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Nov. 2, 2016, 6:08 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
>  a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  a2499040d77dbeb9925a529063fd6a492dd57cc4 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 33)


thrift calls



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 36)


Need to insert  marjer between paragraphs



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 38)


s/less/no more/



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 39)


Why is this link here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 40)


s/firstly/first/



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 57)


Please add docstring with parameters spec - in particular it should mention 
that conf arg is used to get RPC retry count plus any parameters for 
SentryPolicyServiceClientDefaultImpl.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 66)


The comment tells what this method does when connection fails but doesn't 
explain what it does when everything is ok.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 67)


replace 'with' with 'no more'



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 74)


Can this code be executed concurrently? Will it handle isCOnnected logic 
correctly in this case?

I think it is better to put this logic for if(!isConnected) 
connectWithRetry inside client which is synchronized.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 75)


I think it is cleaner to handle exceptions from connectWithRetry() and 
method.invoke() separately.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 80)


Can we get IOException from method.invoke()?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 81)


connectWithRetry() can throw IOExxception - shouldn't you retry in this 
case?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 89)


It may be cleaner if you start with the opposite case:

if (!(targetException instanceof SentryUserException) {
  throw e;
}
handle SentryUserException



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 95)


Should we log something here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 96)


Why not cast to Exception?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 97)


Same thing - you can revert the condition and eliminate else case altogether



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 100)


Shouldn't it be casted to Exception?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 110)


It might be better to say something like

"failed after %d retries: %s"



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/R

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Nov. 2, 2016, 6:08 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Changes
---

Addressed Alexander's comments


Repository: sentry


Description
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
 a41be7fea2c77d0e1f0bbec3e215a33c2cd89417 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 a2499040d77dbeb9925a529063fd6a492dd57cc4 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 214)


I see. sure.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 235)


sure. I will removing the sleep and add a todo here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 49)


i see. sure.


- Li Li


On Oct. 27, 2016, 4:41 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 27, 2016, 4:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  f98ebd135a383ff090b1b204cc62644403310df7 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

> On Nov. 1, 2016, 6:26 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 192
> > 
> >
> > Sure. I think it is better to add metrics in a separate jira / patch, 
> > so I will add a todo here.

Sounds good.


> On Nov. 1, 2016, 6:26 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 216
> > 
> >
> > actually, the current log fomat also contains the exception message:
> > LOGGER.debug(String.format("Got IOException while connecting to %s: ", 
> > addr.toString()), e);

I see. Is it useful to show this Got IOException here since we'll see the full 
exception anyway.


> On Nov. 1, 2016, 6:26 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java,
> >  line 49
> > 
> >
> > Since client connection is expensive, I think we should connect here. 
> > Also it is same as the current implementation of non pool mode. 
> > If connectWithRetry() fails, it will throw exception to sentry client 
> > during creating and opening a new Sentry Service thrift client.

Hmm, what I mean is that we can connect on the first attempt to send a request 
rather than in constructor. Constructor failure is bad, while failure to 
connect is non-fatal.


> On Nov. 1, 2016, 6:26 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java,
> >  line 237
> > 
> >
> > Good catch! Will add a todo here

For now I would suggest just removing the sleep until we figure out what to do 
better.


- Alexander


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


On Oct. 27, 2016, 4:41 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 27, 2016, 4:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  f98ebd135a383ff090b1b204cc62644403310df7 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 190)


Sure. I think it is better to add metrics in a separate jira / patch, so I 
will add a todo here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 198)


throw exception because it is the same as the current implemention.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 204)


sure. I will update it in the new patch



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 212)


yes.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 214)


actually, the current log fomat also contains the exception message:
LOGGER.debug(String.format("Got IOException while connecting to %s: ", 
addr.toString()), e);



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 220)


right. Will use return instead of break, and remove this line.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 235)


Good catch! Will add a todo here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 237)


make senses, will throw runtimeexception for this case



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 36)


yes



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 43)


yes



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 49)


Since client connection is expensive, I think we should connect here. Also 
it is same as the current implementation of non pool mode. 
If connectWithRetry() fails, it will throw exception to sentry client 
during creating and opening a new Sentry Service thrift client.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 53)


yes



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 60)


For non pool model, I think we only need maintain one client connection.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 (line 238)


function here is RPC from client
max retry here is per RPC.
Will add more comments


- Li Li


On Oct. 27, 2016, 4:41 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 27, 2016, 4:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apach

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 190)


Can we have some metrics which show how many errors we got per client? And 
how many successfull connects as well as total number of retries.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 198)


Do we need to throw exception or we can return error instead?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 204)


Can we simplify the code by using 

for (int retryCOunt = 0; retryCount < max; retryCOunt++) ... ?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 212)


Can we just return here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 214)


Exception may contain useful information about the reason for failure. SO 
I'd suggest logging something like

"failed to connecto to %s: %s", addr.toString(), e.message



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 220)


This wouldn't be needed if we return after connect()



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 235)


Note that we are holding clinet lock while sleeping - this doesn't look 
good.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 237)


Why are we ignoring InterruptedException? Someone has a reason to interrupt 
us so we should honor it.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 36)


Can it be package-private instead of public?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 43)


Can it be package-private instead of public?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 49)


Do we need to connect here rather than when we call invoke()? What happens 
when connectWithRetry() fails?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 53)


Can it be package-private instead of public?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 (line 60)


This will serialize with the client. Instead we may create a new client and 
initialize it until we have one that works.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 (line 238)


What does "function" mean here?
Please add comment explaining what this max retry means - is it per 
connection attempt or for lifetime? Also please add a comment explaining what 
happens after retries.

ALternatively you may put a big block comment explaining retry logic in one 
of the files and reference it here.


- Alexander Kolbasov


On Oct. 27, 2016, 4:41 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 27, 2016, 4:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clien

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 116)


Nit: If port is not specified, the value if defaultPort will be used.

Please add docstring for each parameter.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 119)


I would consider packaging host and port in a simple class so that we don't 
have to correlate them by array indices.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 122)


Please add assert that length of all arrays is the same.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 125)


How do you handle IllegalArgumentException?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 (line 71)


Please provide more details - what retries are you talking about. It may be 
best if you describe the retry logic at the class comment and then mention 
tuneables there, so you don't have to comment much for specific fields.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 (line 160)


This comment isn't very clear. It needs a bit more context.


- Alexander Kolbasov


On Oct. 27, 2016, 4:41 p.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 27, 2016, 4:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Since currently non pool model is used for sentry clients, here update retry 
> logic for non-pool model. For each full retry we will cycle through all 
> available sentry servers randomly. After each full retry, we will have a 
> small random sleep.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  730bfec98a78ac11f1fae8ab84f9e6715e802a40 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
>  b7d2be153576f80aa1c0fd230d29523cf6a047c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  f98ebd135a383ff090b1b204cc62644403310df7 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
>  3f57a003903961a6aea98bd583a14b65bd2b98a2 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> Tested in my dev with the following cases:
> 1. one server is down before client connection
> 2. one server is down after client connection and during client request
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Oct. 27, 2016, 4:41 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description (updated)
---

Since currently non pool model is used for sentry clients, here update retry 
logic for non-pool model. For each full retry we will cycle through all 
available sentry servers randomly. After each full retry, we will have a small 
random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 730bfec98a78ac11f1fae8ab84f9e6715e802a40 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 b7d2be153576f80aa1c0fd230d29523cf6a047c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 f98ebd135a383ff090b1b204cc62644403310df7 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 3f57a003903961a6aea98bd583a14b65bd2b98a2 

Diff: https://reviews.apache.org/r/52526/diff/


Testing
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Oct. 14, 2016, 5:06 p.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description (updated)
---

Since currently only pool model is used, here update retry logic for pool 
model. For each full retry we will cycle through all available sentry servers 
randomly. After each full retry, we will have a small random sleep.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 842d5cafb06910fcbe6c53002f2101ec5b890a9e 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 f98ebd135a383ff090b1b204cc62644403310df7 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 

Diff: https://reviews.apache.org/r/52526/diff/


Testing (updated)
---

Tested in my dev with the following cases:
1. one server is down before client connection
2. one server is down after client connection and during client request


Thanks,

Li Li

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 178)


this is one of the constructor in this class.
Do you mean add rand as a memeber var in this class?
but rand is only used in this constructor.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 118)


yes. Good point!



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 (line 146)


There are two different modes for clients, PoolClientInvocationHandler is 
used for pool mode, while SentryPolicyServiceClientDefaultImpl is for non-pool 
mode.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 (line 178)


There are two modes for clients and we may not combine the retry logic 
together. Here in the pool mode, all the availble connections will be added in 
a pool, which will be shared by multiple threads. And all threads will share 
the state of the connections, which is curFreshestEndpointIdx and endpoints in 
the code. If thread A failed to connect to nth server, then thread B after A 
will skip n and try n+1.


- Li Li


On Oct. 7, 2016, 12:30 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 7, 2016, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add retry logic for non-pool model.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  842d5cafb06910fcbe6c53002f2101ec5b890a9e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> In non-pool model, for each full retry we will cycle through all available 
> sentry servers. Before each full retry, we will shuffle the server list, and 
> after each full retry, we will have a small random sleep.
> 
> 
> Thanks,
> 
> Li Li
> 
>

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 137)


Please provide more details in the comment explaining the retry logic, 
random shuffling, delays, etc.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 171)


Here we are not initiating an endpoint, just adding it to the list, so the 
message may say something like "Added server endpoint ..."



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 178)


rand should be set in constructor, not here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 184)


I would suggest a more detailed comment. Something like

"Reorder endpoints randomly to prevent all clients connecting to the same 
endnote at the same time after a node failure"



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 185)


Is it efficient to shuffle a linked list?
Do we need to shuffle if len(endpoints) < 3?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 192)


Add a log message showing connection error



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 (line 209)


The comment isn't quite correct - a more accurate one would be

"Sleep for random number of milliseconds between 1 and maxSleepms + 1. The 
random sleep is introduced to prevent multiple clients connecting to the same 
server at the same time.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 (line 118)


Do we include guava with clients? If yes, consider using 
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/net/HostAndPort.html



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 (line 146)


Why is this code in two different places?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 (line 178)


Why is the logic repeated here?


- Alexander Kolbasov


On Oct. 7, 2016, 12:30 a.m., Li Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52526/
> ---
> 
> (Updated Oct. 7, 2016, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
> Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add retry logic for non-pool model.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  4f42a51b1449fe15f856ba252103e66383e175d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
>  3a96d0b124c00efc99cef256c72c25f5c6168007 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
>  842d5cafb06910fcbe6c53002f2101ec5b890a9e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  abc3f58d21bb774427a34399b6e9f51a37ba51db 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
>  5b0e12bbf12510d8d424aa2b7f51076a913234c5 
> 
> Diff: https://reviews.apache.org/r/52526/diff/
> 
> 
> Testing
> ---
> 
> In non-pool model, for each full retry we will cycle through all available 
> sentry servers. Before each full retry, we will shuffle the server list, and 
> after each full retry, we will h

Re: Review Request 52526: SENTRY-1477: Sentry clients should retry with another server when they get connection errors

[Li Li]

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

(Updated Oct. 7, 2016, 12:30 a.m.)


Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya 
Tirukkovalur.


Repository: sentry


Description
---

Add retry logic for non-pool model.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 4f42a51b1449fe15f856ba252103e66383e175d7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java
 3a96d0b124c00efc99cef256c72c25f5c6168007 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 842d5cafb06910fcbe6c53002f2101ec5b890a9e 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 abc3f58d21bb774427a34399b6e9f51a37ba51db 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 5b0e12bbf12510d8d424aa2b7f51076a913234c5 

Diff: https://reviews.apache.org/r/52526/diff/


Testing (updated)
---

In non-pool model, for each full retry we will cycle through all available 
sentry servers. Before each full retry, we will shuffle the server list, and 
after each full retry, we will have a small random sleep.


Thanks,

Li Li