----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59566/#review176372 -----------------------------------------------------------
Please address the review comments. Functionally the code looks good. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java Lines 33 (patched) <https://reviews.apache.org/r/59566/#comment249756> This is a constant and is used for both UpdateableAuthzPaths and UpdateableAuthzPermissions. You may want to have it common place. I have a suggesgtion, ServiceConstants in org.apache.sentry.hdfs. 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/#comment249796> 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. - kalyan kumar kalvagadda On May 30, 2017, 7:08 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59566/ > ----------------------------------------------------------- > > (Updated May 30, 2017, 7:08 p.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/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/2/ > > > Testing > ------- > > TestHDFSIntegrationEnd2End > > > Thanks, > > Na Li > >
