----------------------------------------------------------- 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) <https://reviews.apache.org/r/52526/#comment223951> 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) <https://reviews.apache.org/r/52526/#comment223982> 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) <https://reviews.apache.org/r/52526/#comment223952> 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) <https://reviews.apache.org/r/52526/#comment223953> yes. 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/#comment223954> 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) <https://reviews.apache.org/r/52526/#comment223955> 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) <https://reviews.apache.org/r/52526/#comment223983> 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) <https://reviews.apache.org/r/52526/#comment223985> 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) <https://reviews.apache.org/r/52526/#comment223962> yes sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 43) <https://reviews.apache.org/r/52526/#comment223969> yes sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 49) <https://reviews.apache.org/r/52526/#comment223967> 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) <https://reviews.apache.org/r/52526/#comment223972> yes sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 60) <https://reviews.apache.org/r/52526/#comment223975> 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) <https://reviews.apache.org/r/52526/#comment223978> 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/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 > >
