You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2016/11/01 00:23:40 UTC
Re: Review Request 52526: SENTRY-1477: Sentry clients should retry
with another server when they get connection errors
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/52526/#comment223874>
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)
<https://reviews.apache.org/r/52526/#comment223869>
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)
<https://reviews.apache.org/r/52526/#comment223856>
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)
<https://reviews.apache.org/r/52526/#comment223852>
Can we just return here?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 214)
<https://reviews.apache.org/r/52526/#comment223851>
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)
<https://reviews.apache.org/r/52526/#comment223855>
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)
<https://reviews.apache.org/r/52526/#comment223876>
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)
<https://reviews.apache.org/r/52526/#comment223872>
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)
<https://reviews.apache.org/r/52526/#comment223837>
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)
<https://reviews.apache.org/r/52526/#comment223838>
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)
<https://reviews.apache.org/r/52526/#comment223875>
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)
<https://reviews.apache.org/r/52526/#comment223839>
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)
<https://reviews.apache.org/r/52526/#comment223877>
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)
<https://reviews.apache.org/r/52526/#comment223844>
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 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
>
>