hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Sichi" <jsi...@fb.com>
Subject Re: Review Request: HIVE-78 patch 9
Date Fri, 17 Dec 2010 20:52:57 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message