----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54729/#review161110 -----------------------------------------------------------
Fix it, then Ship it! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java (line 82) <https://reviews.apache.org/r/54729/#comment232348> iWhat is the point of multiplying 31 by one? Can you add some comment explaining the hash function? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java (line 64) <https://reviews.apache.org/r/54729/#comment232349> Can we pre-create both serializers as static final fields? This way we don't need to create them for every serde op. If we do that, we can just expose toString/fromString methodfs here. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java (line 45) <https://reviews.apache.org/r/54729/#comment232350> Please change this to proper Javadoc and add description for parameters and return values. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java (line 26) <https://reviews.apache.org/r/54729/#comment232351> Nit: need a space between < and Hive, otherwise it doesn't show as '<' sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java (line 28) <https://reviews.apache.org/r/54729/#comment232353> It would be nice to show some example of permission change JSON object. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java (line 29) <https://reviews.apache.org/r/54729/#comment232352> Nit: add <p> - Alexander Kolbasov On Jan. 10, 2017, 1:57 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54729/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2017, 1:57 a.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Vamsee Yarlagadda. > > > Repository: sentry > > > Description > ------- > > Create schema for storing HMS path change and Sentry permission change. It > contains change ID, timestamp and the change event in JSON format. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > 98349232bc658c39791e58b64949ecb975fff7a0 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java > b66f70ba3f9cec6b44dc853ed09494be37303e2f > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > 4dc3a0cebdff89ee2f9070e4d822a28dbd164c08 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java > 53243b44329997e64ab70db5e5d8c3614ab974d6 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > ea1c8f62f300abd03206fdd9ec76fbf216fc6a2d > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java > 315d4b3a80b9ab3ac8704ecbab81876f141423f1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > fb5470f635e03c762beb5bc2c6b06641b2476ce9 > > Diff: https://reviews.apache.org/r/54729/diff/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >