----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62107/#review184945 -----------------------------------------------------------
The approach is fine, the comments below are mostly about documentation about thread-safety of the state bank. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 105 (original), 107 (patched) <https://reviews.apache.org/r/62107/#comment261153> Sequence numbers do not share namespace with notification IDs, so this might confuse readers sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 281 (patched) <https://reviews.apache.org/r/62107/#comment261156> As I learned these asserts do not do anything at all since they are usually never enabled, so Preconditions are better - they will be triggered in testing. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java Lines 17 (patched) <https://reviews.apache.org/r/62107/#comment261157> Please provide more documentation about these - what are these states and what is their semantics. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java Lines 17 (patched) <https://reviews.apache.org/r/62107/#comment261158> Please provide more documentation about these states and their semantics. This is public API for these states. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java Lines 18 (patched) <https://reviews.apache.org/r/62107/#comment261159> This is a publi cgeneric interface, please document what it is and how it should be used, as long as document interface getValue() method. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java Lines 27 (patched) <https://reviews.apache.org/r/62107/#comment261160> Please document API and thread safety. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java Lines 28 (patched) <https://reviews.apache.org/r/62107/#comment261164> make class final? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java Lines 31 (patched) <https://reviews.apache.org/r/62107/#comment261161> Someone may think that this class is thread safe, while it isn't. Is it worth making it completely thread-safe given that it is a generic utility class? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java Lines 53 (patched) <https://reviews.apache.org/r/62107/#comment261162> This part isn;t thread safe since it does non-atomic read/modify/write - Alexander Kolbasov On Sept. 7, 2017, 8:43 p.m., Brian Towles wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62107/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2017, 8:43 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio > Pena, and Vadim Spector. > > > Bugs: SENTRY-1918 > https://issues.apache.org/jira/browse/SENTRY-1918 > > > Repository: sentry > > > Description > ------- > > Check the state if the SentryService to see if its doing a full update and > delay the NN requests for a full update while thats going on. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > 5d744214e14d6c48194b3a0bf84d6e10070b020a > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > 8fbc10048003ab4b8a38676e241ae0fd27d2392c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > c234eafb430b58bfd1e739847e83937a5e34d878 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > d44abffc199cd46d3ab7d2230dfdb2075736a8f0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryStateBankTestHelper.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryStateBank.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/62107/diff/6/ > > > Testing > ------- > > Unit Tests > > > Thanks, > > Brian Towles > >
