> On July 19, 2018, 2:55 a.m., Na Li wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java > > Lines 70 (patched) > > <https://reviews.apache.org/r/67949/diff/1/?file=2061162#file2061162line70> > > > > what's the reason to have allStates? We already have states. > > > > It seems it is only for testing. > > > > Can you not add a variable only for testing? Instead, you can modify > > MockHMSClientFactory and let its function wait for a signal before proceed. > > In this way, you can test when full snapshot is in progress and after full > > snapshot is done.
Currently SentryStatebank only store only one state. There could be cases where we would like to know the previous states as well. "allStates" holds all the states that the cmpenent has moved in the part as well. > On July 19, 2018, 2:55 a.m., Na Li wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java > > Line 114 (original), 118 (patched) > > <https://reviews.apache.org/r/67949/diff/1/?file=2061162#file2061162line118> > > > > You don't update allStates here idea is hold older states as well that is whay i'm not updating the state on disable. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67949/#review206232 ----------------------------------------------------------- On July 17, 2018, 10:53 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67949/ > ----------------------------------------------------------- > > (Updated July 17, 2018, 10:53 p.m.) > > > Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2307 > https://issues.apache.org/jira/browse/SENTRY-2307 > > > Repository: sentry > > > Description > ------- > > Sentry tries to synchronize the HMS operations to make sure that sentry > server process the notifications for create/drop/alter table and databases > before it receives permission grants/revokes on them. Blocking HMS threads to > make sentry processes appropriate notifications makes sense but doing the > same when full snapshot is taken doesn’t serve any purpose as sentry server > doesn’t update sentry permissions based on HMS full snapshot. We can document > that there sentry could have some stale permissions when there are any DDL > operations performed at the time of snapshot creation. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 2efc8cf9c1380a063c54d6bf4ef83e9d0fa8ebc9 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializerState.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > 12266cb25ea70753e07d27b53de777f347f68844 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java > 2c05d49d7f1b7e43318dcfeba22f4f8f4fa3c724 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > 3475624e35cb011df01cc0fa633c01141a882337 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java > 38668ca88f155e0fa2f3beb4db32b18e953e58b6 > > > Diff: https://reviews.apache.org/r/67949/diff/1/ > > > Testing > ------- > > Added tests to verify the changes added. > > > Thanks, > > kalyan kumar kalvagadda > >
