----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/183/#review75 -----------------------------------------------------------
http://svn.apache.org/repos/asf/hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <https://reviews.apache.org/r/183/#comment66> From these parameter names, it's not obvious that they are default grants. http://svn.apache.org/repos/asf/hive/trunk/conf/hive-default.xml <https://reviews.apache.org/r/183/#comment100> This interface has been renamed. http://svn.apache.org/repos/asf/hive/trunk/conf/hive-default.xml <https://reviews.apache.org/r/183/#comment67> Why is your patch adding this irrelevant property? It seems to have sneaked in from some other patch. http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/183/#comment68> This should be granteeName http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/183/#comment69> This should be granteeType http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/183/#comment70> Do you mean properties associated with the role? http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/183/#comment71> Shouldn't this take a Role object so we can pass parameters? http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/183/#comment72> Fix whitespace on these two lines http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/183/#comment73> Since we're not using this call yet, eliminate it from the patch. http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/183/#comment74> You could eliminate even more code by pushing down the type-specific dispatch below the executeWithRetry level. http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/183/#comment75> See comment above about eliminating more code by pushing down below executeWithRetry. http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/183/#comment76> You duplicated the same code here three times instead of using a helper method. http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/183/#comment77> Why is this using RuntimeException? http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/183/#comment78> ditto on RuntimeException http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/183/#comment79> A redundant privilege should be ignored; it should not cause an exception. (If it has grant option and the old one didn't, then the privilege should be upgraded to have the grant option.) http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment81> There should be a unique index on ROLE_NAME http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment80> IS_ROLE/IS_GROUP no longer exists; this should be PRINCIPAL_TYPE. Also, where is the grantor? http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment88> rename to ROLE_GRANT_ID http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment82> Add a unique index: PRINCIPAL_TYPE, PRINCIPAL_NAME, USER_PRIV, GRANTOR http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment89> rename to USER_GRANT_ID http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment86> Since now we're only storing one privilege per row, this and others should be 128 instead of 4000 http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment83> This index should be unique, and it should include PRINCIPAL_TYPE, DB_PRIV, and GRANTOR http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment90> rename to DB_GRANT_ID http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment84> need a unique index http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment91> rename to TABLEPART_GRANT_ID http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment92> rename to COLUMN_GRANT_ID http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo <https://reviews.apache.org/r/183/#comment85> Need a unique index http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java <https://reviews.apache.org/r/183/#comment95> As I noted in JIRA previously, it does not make sense to have doAuthorization be a boolean method which also throws an exception. And here we disregard the boolean return anyway. http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/183/#comment93> Please don't include all of these spurious reformattings in your patch. http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/Authenticator.java <https://reviews.apache.org/r/183/#comment98> As previously noted in JIRA, call this HiveAuthenticationProvider and make it extend Configurable. And add Javadoc. http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/AuthenticatorFactory.java <https://reviews.apache.org/r/183/#comment99> As previously noted in JIRA, get rid of this unnecessary factory class and follow the classloading pattern in HiveUtils instead. http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationManagerFactory.java <https://reviews.apache.org/r/183/#comment94> Per my previous comments, please remove this unneeded factory class and follow the classloading pattern in HiveUtils. http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProvider.java <https://reviews.apache.org/r/183/#comment96> As noted previously in JIRA, this should implement Configurable. http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProvider.java <https://reviews.apache.org/r/183/#comment97> For all methods here, see my comment in Driver about the inconsistency between returning boolean and throwing. Also avoid using arrays in interfaces unless necessary for performance; use List instead. - John On 2010-12-17 12:52:39, John Sichi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/183/ > ----------------------------------------------------------- > > (Updated 2010-12-17 12:52:39) > > > Review request for hive. > > > Summary > ------- > > Review by JVS. Note that the patch had some conflicts, so I wasn't able to > test this version; I'll do more testing and commenting after the next patch. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > 1050266 > http://svn.apache.org/repos/asf/hive/trunk/conf/hive-default.xml 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/if/hive_metastore.thrift > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MColumnPrivilege.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MDBPrivilege.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MGlobalPrivilege.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MRole.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MRoleMap.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/org/apache/hadoop/hive/metastore/model/MTablePartitionPrivilege.java > PRE-CREATION > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/model/package.jdo > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/SMBMapJoinOperator.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/AuthorizationException.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/DDLWork.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/GrantDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/GrantRevokeRoleDDL.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PrincipalDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PrivilegeDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/PrivilegeObjectDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/RevokeDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/RoleDDLDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/ShowGrantDesc.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/Authenticator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/AuthenticatorFactory.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationManagerFactory.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProvider.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/Privilege.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/PrivilegeRegistry.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/PrivilegeScope.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/CreateTableAutomaticGrant.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/QTestUtil.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestSemanticAnalyzerHookLoading.java > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/DummyAuthenticator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_1.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_2.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_3.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_4.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_5.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_6.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_fail_7.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientnegative/authorization_part.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientpositive/authorization_1.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientpositive/authorization_2.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientpositive/authorization_3.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientpositive/authorization_4.q > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientpositive/input19.q > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/queries/clientpositive/show_indexes_edge_cases.q > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_1.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_2.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_3.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_4.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_5.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_6.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_fail_7.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientnegative/authorization_part.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/alter4.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/authorization_1.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/authorization_2.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/authorization_3.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/authorization_4.q.out > PRE-CREATION > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/bucket_groupby.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/create_default_prop.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/ctas.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/input19.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/merge3.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/query_result_fileformat.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/rcfile_default_format.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/semijoin.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/show_indexes_edge_cases.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats10.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats12.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats13.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats2.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats5.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats6.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats7.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats8.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/stats9.q.out > 1050266 > > http://svn.apache.org/repos/asf/hive/trunk/ql/src/test/results/clientpositive/str_to_map.q.out > 1050266 > > Diff: https://reviews.apache.org/r/183/diff > > > Testing > ------- > > > Thanks, > > John > >
