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