Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change tables.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56952/#review167177 --- Ship it! Ship It! - Alexander Kolbasov On Feb. 28, 2017, 7:35 p.m., Lei Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56952/ > --- > > (Updated Feb. 28, 2017, 7:35 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-1637 > https://issues.apache.org/jira/browse/SENTRY-1637 > > > Repository: sentry > > > Description > --- > > * Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable > the background clean thread. > * Use ```ScheduledExecutorSerivce``` to periodically involke > ```SentryStore.purgeDeltaChanges()``` > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > db55b5aa33cebbef235cceca6ccda48603da2a26 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java > 1cce1fc4b9157c76c68b35a292263c5e96270bd2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java > 691c1fb81cd50b6bf671576b3bba69aff291c008 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > c91051db70a5f606980fb29f780fbc199945e4f3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java > a3bb6ab19de35d3018f1e9a938936b22d5aecb48 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > e6021f182e26f7b15773d64d84263c6da586d7a9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 806d03e81a3660a30c6513efbddd2a1610359fc1 > > Diff: https://reviews.apache.org/r/56952/diff/ > > > Testing > --- > > mvn test -Dtest=TestSentryStore > > > Thanks, > > Lei Xu > >
Re: Review Request 57065: SENTRY-1600 Define Thrift API for HMS to Sentry notification barrier
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57065/#review167157 --- Ship it! Ship It! - Hao Hao On Feb. 25, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57065/ > --- > > (Updated Feb. 25, 2017, 7:35 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and > Vamsee Yarlagadda. > > > Bugs: SENTRY-1600 > https://issues.apache.org/jira/browse/SENTRY-1600 > > > Repository: sentry > > > Description > --- > > SENTRY-1600 Define Thrift API for HMS to Sentry notification barrier > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/SentryPolicyService.java > 521a7dbaade0312b0d7bb168379bfb227d5104e0 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentrySyncIDRequest.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentrySyncIDResponse.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 4075e3a049773795ecbd40a0293505bb4cc3b317 > > Diff: https://reviews.apache.org/r/57065/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/ --- (Updated Feb. 28, 2017, 9:30 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector. Changes --- Addressed sasha's review comments. Bugs: SENTRY-1556 https://issues.apache.org/jira/browse/SENTRY-1556 Repository: sentry Description --- SENTRY-1556 Simplify privilege cleaning I made changes for the first approach by removing privileges moment they are not associated to any role. I have identified below scenarios which this will happen When a role is deleted, we can see if the associated privileges are associated to any other roles. All the privileges that are not associated to any roles can be deleted from storage When a privilege is revoked for a role, we can remove the privilege from storage if it is not associated to any role Note: Once this approached is reviewed and accepted, we need not call PrivCleaner periodically for cleanup Diffs (updated) - sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java f9fb0f3 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 27dbfca sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java f1d7a86 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6d54748 sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java 29134fe sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 20ce392 Diff: https://reviews.apache.org/r/54947/diff/ Testing --- Added unit tests the scenarios mentioned in description. Also tested the same with Sentry thrift client. Run unit test cases of TestSentryStore. Thanks, kalyan kumar kalvagadda
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review167142 --- - kalyan kumar kalvagadda On Feb. 28, 2017, 9:30 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > --- > > (Updated Feb. 28, 2017, 9:30 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Bugs: SENTRY-1556 > https://issues.apache.org/jira/browse/SENTRY-1556 > > > Repository: sentry > > > Description > --- > > SENTRY-1556 Simplify privilege cleaning > > I made changes for the first approach by removing privileges moment they are > not associated to any role. > I have identified below scenarios which this will happen > When a role is deleted, we can see if the associated privileges are > associated to any other roles. All the privileges that are not associated to > any roles can be deleted from storage > When a privilege is revoked for a role, we can remove the privilege from > storage if it is not associated to any role > > Note: Once this approached is reviewed and accepted, we need not call > PrivCleaner periodically for cleanup > > > Diffs > - > > > sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > f9fb0f3 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 27dbfca > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > f1d7a86 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 6d54748 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > 29134fe > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 20ce392 > > Diff: https://reviews.apache.org/r/54947/diff/ > > > Testing > --- > > Added unit tests the scenarios mentioned in description. > Also tested the same with Sentry thrift client. > Run unit test cases of TestSentryStore. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 57065: SENTRY-1600 Define Thrift API for HMS to Sentry notification barrier
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57065/#review167146 --- Ship it! Ship It! - kalyan kumar kalvagadda On Feb. 25, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57065/ > --- > > (Updated Feb. 25, 2017, 7:35 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Sergio Pena, and > Vamsee Yarlagadda. > > > Bugs: SENTRY-1600 > https://issues.apache.org/jira/browse/SENTRY-1600 > > > Repository: sentry > > > Description > --- > > SENTRY-1600 Define Thrift API for HMS to Sentry notification barrier > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/SentryPolicyService.java > 521a7dbaade0312b0d7bb168379bfb227d5104e0 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentrySyncIDRequest.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentrySyncIDResponse.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 4075e3a049773795ecbd40a0293505bb4cc3b317 > > Diff: https://reviews.apache.org/r/57065/diff/ > > > Testing > --- > > > Thanks, > > Alexander Kolbasov > >
Re: Review Request 56952: SENTRY-1637. Periodically purge Delta change tables.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56952/ --- (Updated Feb. 28, 2017, 11:35 a.m.) Review request for sentry. Changes --- Add comments to required fields, to address reviews from Sasha. Bugs: SENTRY-1637 https://issues.apache.org/jira/browse/SENTRY-1637 Repository: sentry Description --- * Add configuration keys: SENTRY_STORE_DELTA_REMOVAL_PERIOD_SECONDS to enable the background clean thread. * Use ```ScheduledExecutorSerivce``` to periodically involke ```SentryStore.purgeDeltaChanges()``` Diffs (updated) - sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java db55b5aa33cebbef235cceca6ccda48603da2a26 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorFactory.java 1cce1fc4b9157c76c68b35a292263c5e96270bd2 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 7fc3ca8bc2444f6cfcbcbabaebf0d3e18ef3d209 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessorFactory.java 691c1fb81cd50b6bf671576b3bba69aff291c008 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java c91051db70a5f606980fb29f780fbc199945e4f3 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java a3bb6ab19de35d3018f1e9a938936b22d5aecb48 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java e6021f182e26f7b15773d64d84263c6da586d7a9 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 806d03e81a3660a30c6513efbddd2a1610359fc1 Diff: https://reviews.apache.org/r/56952/diff/ Testing --- mvn test -Dtest=TestSentryStore Thanks, Lei Xu
Re: Review Request 54798: SENTRY-1546 Generic Policy provides bad error messages for Sentry exceptions
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54798/ --- (Updated Feb. 28, 2017, 5:57 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector. Changes --- Addressed review comments Bugs: SENTRY-1548 https://issues.apache.org/jira/browse/SENTRY-1548 Repository: sentry Description --- Added code changes to SentryStore to have proper message (where the exceptions are originated). If this is done we can avoid decorating them later. I saw issue with exceptions created while role create/drop and made required changes. Diffs (updated) - sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryAlreadyExistsException.java f29c42f sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryNoSuchObjectException.java 70719e4 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 27dbfca sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7b926a5 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2 sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java 3881c11 sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 9b986d7 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 0239388 Diff: https://reviews.apache.org/r/54798/diff/ Testing --- I have performed tests using Sentrytool Client. Thanks, kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54454/ --- (Updated Feb. 28, 2017, 2:21 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector. Changes --- Changed th branch and Bug fields. Bugs: SENTRY-1548 https://issues.apache.org/jira/browse/SENTRY-1548 Repository: sentry Description --- SENTRY-1548 Setting GrantOption to UNSET upsets Sentry I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them. Diffs - sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION Diff: https://reviews.apache.org/r/54454/diff/ Testing --- Verfied the changes using sentry thrift client. Thanks, kalyan kumar kalvagadda
Re: Review Request 54798: SENTRY-1546 Generic Policy provides bad error messages for Sentry exceptions
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54798/ --- (Updated Feb. 28, 2017, 2:20 p.m.) Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector. Bugs: SENTRY-1548 https://issues.apache.org/jira/browse/SENTRY-1548 Repository: sentry Description --- Added code changes to SentryStore to have proper message (where the exceptions are originated). If this is done we can avoid decorating them later. I saw issue with exceptions created while role create/drop and made required changes. Diffs - sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryAlreadyExistsException.java f29c42f sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryNoSuchObjectException.java 70719e4 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 27dbfca sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7b926a5 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2 sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java 3881c11 sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 9b986d7 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 0239388 Diff: https://reviews.apache.org/r/54798/diff/ Testing --- I have performed tests using Sentrytool Client. Thanks, kalyan kumar kalvagadda
