----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review154867 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 33) <https://reviews.apache.org/r/52526/#comment224629> thrift calls sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 36) <https://reviews.apache.org/r/52526/#comment224630> Need to insert <p> marjer between paragraphs sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 38) <https://reviews.apache.org/r/52526/#comment224638> s/less/no more/ sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 39) <https://reviews.apache.org/r/52526/#comment224631> Why is this link here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 40) <https://reviews.apache.org/r/52526/#comment224634> s/firstly/first/ sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 57) <https://reviews.apache.org/r/52526/#comment224632> Please add docstring with parameters spec - in particular it should mention that conf arg is used to get RPC retry count plus any parameters for SentryPolicyServiceClientDefaultImpl. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 66) <https://reviews.apache.org/r/52526/#comment224639> The comment tells what this method does when connection fails but doesn't explain what it does when everything is ok. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 67) <https://reviews.apache.org/r/52526/#comment224633> replace 'with' with 'no more' sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 74) <https://reviews.apache.org/r/52526/#comment224654> Can this code be executed concurrently? Will it handle isCOnnected logic correctly in this case? I think it is better to put this logic for if(!isConnected) connectWithRetry inside client which is synchronized. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 75) <https://reviews.apache.org/r/52526/#comment224642> I think it is cleaner to handle exceptions from connectWithRetry() and method.invoke() separately. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 80) <https://reviews.apache.org/r/52526/#comment224641> Can we get IOException from method.invoke()? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 81) <https://reviews.apache.org/r/52526/#comment224640> connectWithRetry() can throw IOExxception - shouldn't you retry in this case? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 89) <https://reviews.apache.org/r/52526/#comment224644> It may be cleaner if you start with the opposite case: if (!(targetException instanceof SentryUserException) { throw e; } handle SentryUserException sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 95) <https://reviews.apache.org/r/52526/#comment224649> Should we log something here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 96) <https://reviews.apache.org/r/52526/#comment224647> Why not cast to Exception? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 97) <https://reviews.apache.org/r/52526/#comment224645> Same thing - you can revert the condition and eliminate else case altogether sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 100) <https://reviews.apache.org/r/52526/#comment224646> Shouldn't it be casted to Exception? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 110) <https://reviews.apache.org/r/52526/#comment224651> It might be better to say something like "failed after %d retries: %s" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 111) <https://reviews.apache.org/r/52526/#comment224653> Same here sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java (line 126) <https://reviews.apache.org/r/52526/#comment224656> Note that this will leak file descriptors since you don't close failed endpoints. - Alexander Kolbasov On Nov. 2, 2016, 6:08 p.m., Li Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52526/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2016, 6:08 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/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 > >
