> 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 > > <https://reviews.apache.org/r/52526/diff/8/?file=1555021#file1555021line147> > > > > 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 > > <https://reviews.apache.org/r/52526/diff/8/?file=1555021#file1555021line218> > > > > 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 > > <https://reviews.apache.org/r/52526/diff/8/?file=1555021#file1555021line219> > > > > 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 > > <https://reviews.apache.org/r/52526/diff/8/?file=1555021#file1555021line159> > > > > 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 client connection > 2. one server is down after client connection and during client request > > > Thanks, > > Li Li > >
