You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2018/08/17 17:24:47 UTC
Re: Review Request 67949: SENTRY-2307: Avoid HMS event
synchronization while sentry is fetching full snapshot.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67949/#review207514
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializerState.java
Lines 28 (patched)
<https://reviews.apache.org/r/67949/#comment290921>
It is enum, but only one state. Kind of weird.
Should it has other states, such as "INIT", "SUCCEED", "FAILED"
Or it is just a normal class, no need to be enum
- Na Li
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
>
>
Re: Review Request 67949: SENTRY-2307: Avoid HMS event
synchronization while sentry is fetching full snapshot.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 5:24 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializerState.java
> > Lines 28 (patched)
> > <https://reviews.apache.org/r/67949/diff/1/?file=2061160#file2061160line28>
> >
> > It is enum, but only one state. Kind of weird.
> >
> > Should it has other states, such as "INIT", "SUCCEED", "FAILED"
> >
> > Or it is just a normal class, no need to be enum
"INIT", "SUCCEED", "FAILED" reflect status not current state.
Reason i did not add addtional states as we need not track any other state at this point in time. When we may need to track more states in future we can extend the enum to have more states. What do you say?
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67949/#review207514
-----------------------------------------------------------
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
>
>
Re: Review Request 67949: SENTRY-2307: Avoid HMS event
synchronization while sentry is fetching full snapshot.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Aug. 17, 2018, 5:24 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializerState.java
> > Lines 28 (patched)
> > <https://reviews.apache.org/r/67949/diff/1/?file=2061160#file2061160line28>
> >
> > It is enum, but only one state. Kind of weird.
> >
> > Should it has other states, such as "INIT", "SUCCEED", "FAILED"
> >
> > Or it is just a normal class, no need to be enum
>
> kalyan kumar kalvagadda wrote:
> "INIT", "SUCCEED", "FAILED" reflect status not current state.
> Reason i did not add addtional states as we need not track any other state at this point in time. When we may need to track more states in future we can extend the enum to have more states. What do you say?
I am OK with this.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67949/#review207514
-----------------------------------------------------------
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
>
>