----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review152715 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java (line 116) <https://reviews.apache.org/r/52526/#comment221818> Nit: If port is not specified, the value if defaultPort will be used. Please add docstring for each parameter. 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/#comment221816> I would consider packaging host and port in a simple class so that we don't have to correlate them by array indices. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java (line 122) <https://reviews.apache.org/r/52526/#comment221815> Please add assert that length of all arrays is the same. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java (line 125) <https://reviews.apache.org/r/52526/#comment221814> How do you handle IllegalArgumentException? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 71) <https://reviews.apache.org/r/52526/#comment221819> Please provide more details - what retries are you talking about. It may be best if you describe the retry logic at the class comment and then mention tuneables there, so you don't have to comment much for specific fields. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 160) <https://reviews.apache.org/r/52526/#comment221822> This comment isn't very clear. It needs a bit more context. - 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 > >
