----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18694/#review36031 -----------------------------------------------------------
I only looked over the common, search, and solr parts. Looks good, mostly nits below. sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java <https://reviews.apache.org/r/18694/#comment66838> permission -> privilege in this comment sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java <https://reviews.apache.org/r/18694/#comment66839> Same here sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java <https://reviews.apache.org/r/18694/#comment66840> why is this not getPrivileges? sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SearchWildcardPrivilege.java <https://reviews.apache.org/r/18694/#comment66842> why is this class serializable? sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SearchWildcardPrivilege.java <https://reviews.apache.org/r/18694/#comment66843> permissoin -> privilege in this comment sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SearchWildcardPrivilege.java <https://reviews.apache.org/r/18694/#comment66844> same here sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java <https://reviews.apache.org/r/18694/#comment66836> call this getPrivilegeFactory [I think privilege is a better name anyway :)] sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java <https://reviews.apache.org/r/18694/#comment66846> nit: missing new line? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java <https://reviews.apache.org/r/18694/#comment66847> any reason this is a list and not a set? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java <https://reviews.apache.org/r/18694/#comment66848> it would be helpful to have a comment about what strict means sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ResourceAuthorizationProvider.java <https://reviews.apache.org/r/18694/#comment66852> both of these should be named based on privilege rather than permission. sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ResourceAuthorizationProvider.java <https://reviews.apache.org/r/18694/#comment66859> We expect this class to be thread safe? What other classes do we expect to be thread safe? What is the expected usage? sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java <https://reviews.apache.org/r/18694/#comment66865> probably better to make this line last, so if I pass in a context with some problem (e.g. one of the above functions throws a runtimeexception or something), I can recover. sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java <https://reviews.apache.org/r/18694/#comment66864> should these things be named based on privileges instead of permissions? - Gregory Chanan On March 3, 2014, 4:55 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18694/ > ----------------------------------------------------------- > > (Updated March 3, 2014, 4:55 p.m.) > > > Review request for sentry and Shreepadma Venugopalan. > > > Bugs: SENTRY-122 > https://issues.apache.org/jira/browse/SENTRY-122 > > > Repository: sentry > > > Description > ------- > > The big changes are: > > 1) Push down groups requested to ProviderBackend. This will allow the DB > Backend to pull privileges associated with a set of Groups. > 2) Often the provider was saying "role" when it meant "privilege" in names of > variables and types. I fixed this as it kept confusing me. > 3) We were often using List<String> when we meant Set<String>. > 4) Not as important as #2, but we also used Permission to mean Privilege. I > fixed this to be consistent as well going forward. > 5) I cleaned up a ton of warnings. > > > Diffs > ----- > > bin/sentry 9f2ce77 > pom.xml 06649bd > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 506f185 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingPreExecHook.java > f120c77 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryOnFailureHookContext.java > 2beacd0 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryOnFailureHookContextImpl.java > d8ffe23 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > f6a1ecc > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > b20ec34 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > d7a518d > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/MockUserToGroupMapping.java > 83432ca > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java > ea2c7ea > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestURI.java > 1853559 > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrAuthorizationException.java > 134eaeb > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > c6ce53e > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java > c9ee8ba > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java > b061eec > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryConfigurationException.java > 516b2da > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java > 1659450 > > sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java > 28818ba > > sentry-core/sentry-core-model-search/src/test/java/org/apache/sentry/core/search/TestCollection.java > bc00b62 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PermissionFactory.java > 45fd7bd > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java > c08d082 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java > PRE-CREATION > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java > PRE-CREATION > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeValidator.java > PRE-CREATION > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeValidatorContext.java > PRE-CREATION > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/RoleValidator.java > 8390364 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/AbstractDBPrivilegeValidator.java > PRE-CREATION > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/AbstractDBRoleValidator.java > 722a4be > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBWildcardPermission.java > 01981d1 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBWildcardPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DatabaseMustMatch.java > a7c2091 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DatabaseRequiredInPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DatabaseRequiredInRole.java > 48b36a6 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/ServerNameMustMatch.java > 8ddf1dd > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/ServersAllIsInvalid.java > 9445b0b > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java > 1d01b47 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java > 89ca737 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/DBPolicyFileBackend.java > d8d68b7 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestDBModelAuthorizables.java > 23b03d4 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestDBWildcardPermission.java > 2024cd8 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestDBWildcardPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestDatabaseRequiredInRole.java > 948b7ac > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java > f348e0e > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestResourceAuthorizationProviderGeneralCases.java > 2f4c20e > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestResourceAuthorizationProviderSpecialCases.java > 688b845 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java > c093dde > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineLocalFS.java > 86ec2fa > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/AbstractSearchPrivilegeValidator.java > PRE-CREATION > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/AbstractSearchRoleValidator.java > 8e7c19f > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/CollectionRequiredInPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/CollectionRequiredInRole.java > 7f152d9 > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SearchWildcardPermission.java > 2d2e0bb > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SearchWildcardPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java > 51ab35d > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/AbstractTestSearchPolicyEngine.java > 24e9521 > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/SearchPolicyFileBackend.java > 874f2db > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestCollectionRequiredInRole.java > a56aabd > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchAuthorizationProviderGeneralCases.java > 4bbaf3a > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchAuthorizationProviderSpecialCases.java > 2a7872d > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchModelAuthorizables.java > bd06b7e > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchPolicyEngineDFS.java > 1683eec > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchPolicyNegative.java > 0770aa8 > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchWildcardPermission.java > b20595d > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchWildcardPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java > 4887678 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/GroupMappingService.java > 226cc88 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoAuthorizationProvider.java > 8f18926 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoGroupMappingService.java > e1bc6d2 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java > 327a3a5 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/Roles.java > a8f36a3 > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/MockGroupMappingServiceProvider.java > 806b42e > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestNoAuthorizationProvider.java > 3f48f49 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 549a9db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryConfigurationException.java > 2aeea42 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 55dbcdd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/KerberosConfiguration.java > 8323959 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyStoreProcessor.java > ab4fff6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 692dbfa > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > c1bb887 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/HadoopGroupMappingService.java > 4db465d > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/HadoopGroupResourceAuthorizationProvider.java > ff3adf1 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java > c399117 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupResourceAuthorizationProvider.java > 374e989 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java > bed3202 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFiles.java > 295ce78 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ResourceAuthorizationProvider.java > 1b5f2c2 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java > 9eabb53 > > sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestGetGroupMapping.java > f223bee > > sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestKeyValue.java > 1fd64f1 > > sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestLocalGroupMapping.java > f1d8192 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 6ae3776 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java > 2f83678 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestConfigTool.java > 6968cc0 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPerDBConfiguration.java > 80912a3 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtTableScope.java > c267ea6 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java > 8222590 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/AbstractDFS.java > 1068dbe > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/ClusterDFS.java > 1e2c01e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/DFS.java > b9764bc > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/DFSFactory.java > c3e5bf3 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java > dba2a54 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/EmbeddedHiveServer.java > ce3b97c > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java > 0751e91 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalHiveServer.java > 3a257bf > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/UnmanagedHiveServer.java > 4425efa > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java > 05c5263 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/HdfsTestUtil.java > f68fd28 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/ModifiableUserAuthenticationFilter.java > b7081ba > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestCollAdminCoreOperations.java > 865fd10 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestQueryOperations.java > ace0d0f > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/TestUpdateOperations.java > aaca7b4 > > Diff: https://reviews.apache.org/r/18694/diff/ > > > Testing > ------- > > Unit tests pass. > > > Thanks, > > Brock Noland > >
