> On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote: > > Thanks for picking this up Hao! Appreciate it! Do we need to update the non > > pool java clients as well?
Yeah, good point. Tend to deprecated non pool java clients in this case. Do you know any reason why we have to keep it? > On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java, > > line 107 > > <https://reviews.apache.org/r/50578/diff/2/?file=1456880#file1456880line107> > > > > Is this change in behavior required? Yeah, the expected excpetion now become nested inside. > On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, > > lines 260-268 > > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line260> > > > > Seems like endPointIdx will always be the same as > > curFreshestEndpointIdx? Yeah, curFreshestEndpointIdx is working as a temp variable to set the freshestEndpointIdx value. > On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, > > lines 273-278 > > <https://reviews.apache.org/r/50578/diff/2/?file=1456877#file1456877line273> > > > > It is not very clear what we are trying to achieve here. Seems like the > > order of endpoints matter as per this logic? Here is after reach the retry max, checking for all exception received is it caused by standby or active is unreachable. The order of endpoints does not matter. > On July 28, 2016, 10:18 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java, > > line 26 > > <https://reviews.apache.org/r/50578/diff/2/?file=1456881#file1456881line26> > > > > I only see tests for hostString parsing logic, shall we add tests for > > actual standbyException handling? I think it is not straightforward to do it here, would be better to do in the failover e2e test? - Hao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50578/#review144007 ----------------------------------------------------------- On July 28, 2016, 9:14 p.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50578/ > ----------------------------------------------------------- > > (Updated July 28, 2016, 9:14 p.m.) > > > Review request for sentry and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > The sentry client should retry configurable times RPCs if it gets a > SentryStandbyException > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java > b2df699b962def9ef2dbf60fe8f601f9aaeb03f5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 1039e6ebfde32ec8db82287458b8b4b6a23136a3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > a35bf1d4781c7424283610c7fa67ede990e35fef > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > 56d774b883914dbffff438101f47fb94518c94d5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java > afea78a0bea754f950cc7e3033e1b1ebfdcb35fc > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java > 15eab15b79f9057d173e0ea4ea64e152ade698d6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/50578/diff/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >
