> On July 11, 2017, 5:39 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 79 (original), 72 (patched) > > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line80> > > > > do we still need this variable? There is hiveConnectionFactory to > > create the client.
Yes, we still need this - it is the actual open client. > On July 11, 2017, 5:39 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 106 (original), 99 (patched) > > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line109> > > > > if client.invalidate() is called, should you return false here? Then > > this function should check if client is invalidated. When invalidate() is called, we call `closeHMSConnection` which sets it to false. > On July 11, 2017, 5:39 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 122 (original), 120 (patched) > > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line130> > > > > To be safe, can you check if client is valid? if not, create a new > > client. There is no method to check validity of the client. Users are not supposed to use invalidated or closed clients. > On July 11, 2017, 5:39 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 282 (patched) > > <https://reviews.apache.org/r/60775/diff/1/?file=1774233#file1774233line373> > > > > should you set client = null after close it? Otherwise, previous > > exception could cause client to close, and HMSFollower client does not work > > any more. It is set to null in finally clause > On July 11, 2017, 5:39 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java > > Lines 25 (patched) > > <https://reviews.apache.org/r/60775/diff/1/?file=1774234#file1774234line25> > > > > Maybe in another Jira, can we have a connection pool, so we can reuse > > the connection if it is valid once it is used? That will improve > > performance and make getting full snapshot faster. There is another JIRA open for Hive. If they don't do it may be we'll do it in Sentry, but I'd prefer Hive team to provide it. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60775/#review180207 ----------------------------------------------------------- On July 11, 2017, 3:46 p.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60775/ > ----------------------------------------------------------- > > (Updated July 11, 2017, 3:46 p.m.) > > > Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, > Sergio Pena, and Vamsee Yarlagadda. > > > Bugs: SENTRY-1630 > https://issues.apache.org/jira/browse/SENTRY-1630 > > > Repository: sentry > > > Description > ------- > > SENTRY-1630 out of sequence error in HMSFollower > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > cf9774c4fb5895df1fd3c26b9135ea5cad9344ef > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java > 389e9b8457aa03b44e2ad94c405d3dec7e422de8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 1f7eb18ac9d9dbb3607dc27b26d4eb2e3c595837 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java > 8d78d1d4cadb1fa455a37bc8276793e9f231d691 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > ec938da1fb02bd0e58554336b671d14e7c6fc621 > > > Diff: https://reviews.apache.org/r/60775/diff/1/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
