> On Nov. 1, 2016, 6:26 p.m., Li Li wrote: > > 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/diff/4/?file=1547631#file1547631line192> > > > > Sure. I think it is better to add metrics in a separate jira / patch, > > so I will add a todo here.
Sounds good. > On Nov. 1, 2016, 6:26 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java, > > line 216 > > <https://reviews.apache.org/r/52526/diff/4/?file=1547631#file1547631line216> > > > > actually, the current log fomat also contains the exception message: > > LOGGER.debug(String.format("Got IOException while connecting to %s: ", > > addr.toString()), e); I see. Is it useful to show this Got IOException here since we'll see the full exception anyway. > On Nov. 1, 2016, 6:26 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java, > > line 49 > > <https://reviews.apache.org/r/52526/diff/4/?file=1547634#file1547634line49> > > > > Since client connection is expensive, I think we should connect here. > > Also it is same as the current implementation of non pool mode. > > If connectWithRetry() fails, it will throw exception to sentry client > > during creating and opening a new Sentry Service thrift client. Hmm, what I mean is that we can connect on the first attempt to send a request rather than in constructor. Constructor failure is bad, while failure to connect is non-fatal. > On Nov. 1, 2016, 6:26 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java, > > line 237 > > <https://reviews.apache.org/r/52526/diff/4/?file=1547631#file1547631line237> > > > > Good catch! Will add a todo here For now I would suggest just removing the sleep until we figure out what to do better. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52526/#review154419 ----------------------------------------------------------- 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 > >
