----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review151908 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 137) <https://reviews.apache.org/r/52526/#comment220561> Please provide more details in the comment explaining the retry logic, random shuffling, delays, etc. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 171) <https://reviews.apache.org/r/52526/#comment220563> Here we are not initiating an endpoint, just adding it to the list, so the message may say something like "Added server endpoint ..." sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 178) <https://reviews.apache.org/r/52526/#comment220562> rand should be set in constructor, not here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 184) <https://reviews.apache.org/r/52526/#comment220564> I would suggest a more detailed comment. Something like "Reorder endpoints randomly to prevent all clients connecting to the same endnote at the same time after a node failure" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 185) <https://reviews.apache.org/r/52526/#comment220566> Is it efficient to shuffle a linked list? Do we need to shuffle if len(endpoints) < 3? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 192) <https://reviews.apache.org/r/52526/#comment220567> Add a log message showing connection error sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 209) <https://reviews.apache.org/r/52526/#comment220568> The comment isn't quite correct - a more accurate one would be "Sleep for random number of milliseconds between 1 and maxSleepms + 1. The random sleep is introduced to prevent multiple clients connecting to the same server at the same time. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java (line 118) <https://reviews.apache.org/r/52526/#comment220569> Do we include guava with clients? If yes, consider using https://google.github.io/guava/releases/19.0/api/docs/com/google/common/net/HostAndPort.html sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 146) <https://reviews.apache.org/r/52526/#comment220573> Why is this code in two different places? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java (line 178) <https://reviews.apache.org/r/52526/#comment220574> Why is the logic repeated here? - Alexander Kolbasov On Oct. 7, 2016, 12:30 a.m., Li Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52526/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2016, 12:30 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Hao Hao, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > Add retry logic for non-pool model. > > > 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 > 842d5cafb06910fcbe6c53002f2101ec5b890a9e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 5b0e12bbf12510d8d424aa2b7f51076a913234c5 > > Diff: https://reviews.apache.org/r/52526/diff/ > > > Testing > ------- > > In non-pool model, for each full retry we will cycle through all available > sentry servers. Before each full retry, we will shuffle the server list, and > after each full retry, we will have a small random sleep. > > > Thanks, > > Li Li > >
