> On May 31, 2017, 3:01 p.m., kalyan kumar kalvagadda wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > > Lines 65-79 (original), 64-98 (patched) > > <https://reviews.apache.org/r/59566/diff/2/?file=1734807#file1734807line65> > > > > Functionally this code is fine with me. I have two commments here. > > > > 1. Logic is not clean and is confusing. > > 2. verbose bebug logging is good but having reedundent logging is not > > good. Some of the cases inner functions do appropriate logging. Each log > > should serve some purpose and should help in debugging. > > Here are some cases that I would like to point > > 1. LOGGER.info("#### GetAllUpdatesFrom return full image with {}", > > outMessage); > > This log message would not add any value as it prints the same info in > > lint 104 as well. > > 2. LOGGER.debug("#### GetAllUpdatesFrom will get full image with empty > > delta entries"); > > When some of the delta updates are missing > > "deltaRetriever.retrieveDelta" would log detailed log message. log you have > > added in 94 does not give any usefull info. > > Na Li wrote: > LOGGER.info("#### GetAllUpdatesFrom return full image with {}" is used to > be in pair with line 104 to mark the beginning and end of getting full > snapshot. I will update, so we don't log so many messages.
I add atomic counter to track concurrent requests. So I remove the extra logging message. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59566/#review176372 ----------------------------------------------------------- On June 1, 2017, 2:56 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59566/ > ----------------------------------------------------------- > > (Updated June 1, 2017, 2:56 a.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, > Sergio Pena, and Vamsee Yarlagadda. > > > Bugs: SENTRY-1784 > https://issues.apache.org/jira/browse/SENTRY-1784 > > > Repository: sentry > > > Description > ------- > > HDFS starts requesting changeID = 0 instead of 1. DBUpdateForwarder returns > full update if the request changeID <= 0. After first full update, the > request changeID = 1, so only delta update is sent unless clean up removes > changes that are not sent to HDFS. This fixes both issues in this Jira. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 0741ebc > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java > ad7f8c9 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > 90ba721 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryUpdater.java > 34caa0e > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java > 431c7fe > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > b8542b3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 9beb07b > > > Diff: https://reviews.apache.org/r/59566/diff/3/ > > > Testing > ------- > > TestHDFSIntegrationEnd2End > > > Thanks, > > Na Li > >
