----------------------------------------------------------- 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) <https://reviews.apache.org/r/52526/#comment225201> 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) <https://reviews.apache.org/r/52526/#comment225206> 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) <https://reviews.apache.org/r/52526/#comment225288> nit: use checkNotNull sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 169) <https://reviews.apache.org/r/52526/#comment225291> 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) <https://reviews.apache.org/r/52526/#comment225292> 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) <https://reviews.apache.org/r/52526/#comment225295> 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) <https://reviews.apache.org/r/52526/#comment225297> 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) <https://reviews.apache.org/r/52526/#comment225293> 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) <https://reviews.apache.org/r/52526/#comment225294> 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) <https://reviews.apache.org/r/52526/#comment225296> 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) <https://reviews.apache.org/r/52526/#comment225289> 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) <https://reviews.apache.org/r/52526/#comment225290> How do we handle parsing error? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 82) <https://reviews.apache.org/r/52526/#comment225298> 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) <https://reviews.apache.org/r/52526/#comment225299> 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) <https://reviews.apache.org/r/52526/#comment225300> Adding 'continue' above will get rid of this check - 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 > >
