hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Navis Ryu" <navis....@nexr.com>
Subject Re: Review Request 24043: DefaultHiveAuthorizationProvider should not initialize a new HiveConf
Date Wed, 30 Jul 2014 02:16:25 GMT


> On July 29, 2014, 6:53 p.m., Thejas Nair wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java, line
247
> > <https://reviews.apache.org/r/24043/diff/1/?file=644372#file644372line247>
> >
> >     I think it would be safer to consider a value difference in case to be a different
setting. Eg, some fields like METASTOREPWD,METASTOREDIRECTORY can be case sensitive.
> >     
> >     I realize that equalsIgnoreCase is what old code was doing, but since we are
anyway changing this function, I think it makes sense to fix it.
> >

Nice catch. Missed that. Thanks.


> On July 29, 2014, 6:53 p.m., Thejas Nair wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java,
line 108
> > <https://reviews.apache.org/r/24043/diff/1/?file=644379#file644379line108>
> >
> >     I think INFO or DEBUG level is better here. Also, printing changing it to some
prefix in in the message would be useful. eg -
> >     
> >     LOG.INFO("Current user" + currentUserName + ", Current Roles" + currentRoles);
> >

It's included by mistake. But logging it with info level seemed reasonable.


> On July 29, 2014, 6:53 p.m., Thejas Nair wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java, line 512
> > <https://reviews.apache.org/r/24043/diff/1/?file=644380#file644380line512>
> >
> >     why is this needed ?
> >

Now, if the metastore client inside Hive is not yet connected, new Hive instance will not
be created. It's dummy invocation to connect the metastore client.


- Navis


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24043/#review48996
-----------------------------------------------------------


On July 29, 2014, 7:18 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24043/
> -----------------------------------------------------------
> 
> (Updated July 29, 2014, 7:18 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-6437
>     https://issues.apache.org/jira/browse/HIVE-6437
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> During a HS2 connection, every SessionState got initializes a new DefaultHiveAuthorizationProvider
object (on stock configs).
> 
> In turn, DefaultHiveAuthorizationProvider carries a {{new HiveConf(…)}} that may prove
too expensive, and unnecessary to do, since SessionState itself sends in a fully applied HiveConf
to it in the first place.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java ae7cd53 
>   contrib/src/java/org/apache/hadoop/hive/contrib/metastore/hooks/TestURLHook.java 39562ea

>   contrib/src/test/queries/clientnegative/url_hook.q c346432 
>   contrib/src/test/queries/clientpositive/url_hook.q PRE-CREATION 
>   contrib/src/test/results/clientnegative/url_hook.q.out 601fd93 
>   contrib/src/test/results/clientpositive/url_hook.q.out PRE-CREATION 
>   data/conf/hive-site.xml fe8080a 
>   itests/hive-unit/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java e8d405d 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreVersion.java
0bb022e 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 2fefa06 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 5cc1cd8 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java d26183b

>   metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 5add436 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java 1cf09d4 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 81323f6 
>   ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
2fa512c 
>   ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java
0dfd997 
>   ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveRoleGrant.java
ce07f32 
>   ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java
ce12edb 
>   ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java d218271 
>   ql/src/test/queries/clientnegative/authorization_cannot_create_all_role.q de91e91 
>   ql/src/test/queries/clientnegative/authorization_cannot_create_default_role.q 42a42f6

>   ql/src/test/queries/clientnegative/authorization_cannot_create_none_role.q 0d14cde

>   ql/src/test/queries/clientnegative/authorization_caseinsensitivity.q d5ea284 
>   ql/src/test/queries/clientnegative/authorization_drop_db_cascade.q edeae9b 
>   ql/src/test/queries/clientnegative/authorization_drop_db_empty.q 46d4d0f 
>   ql/src/test/queries/clientnegative/authorization_drop_role_no_admin.q a7aa17f 
>   ql/src/test/queries/clientnegative/authorization_priv_current_role_neg.q 463358a 
>   ql/src/test/queries/clientnegative/authorization_role_cycles1.q a819d20 
>   ql/src/test/queries/clientnegative/authorization_role_cycles2.q 423f030 
>   ql/src/test/queries/clientnegative/authorization_role_grant.q c5c500a 
>   ql/src/test/queries/clientnegative/authorization_role_grant2.q 7fdf157 
>   ql/src/test/queries/clientnegative/authorization_role_grant_nosuchrole.q f456165 
>   ql/src/test/queries/clientnegative/authorization_role_grant_otherrole.q f91abdb 
>   ql/src/test/queries/clientnegative/authorization_role_grant_otheruser.q a530043 
>   ql/src/test/queries/clientnegative/authorization_rolehierarchy_privs.q d9f4c7c 
>   ql/src/test/queries/clientnegative/authorization_set_role_neg2.q 03f748f 
>   ql/src/test/queries/clientnegative/authorization_show_grant_otherrole.q a709d16 
>   ql/src/test/queries/clientnegative/authorization_show_grant_otheruser_all.q 2073cda

>   ql/src/test/queries/clientnegative/authorization_show_grant_otheruser_alltabs.q 672b81b

>   ql/src/test/queries/clientnegative/authorization_show_grant_otheruser_wtab.q 7d95a9d

>   ql/src/test/queries/clientpositive/authorization_1_sql_std.q 381937c 
>   ql/src/test/queries/clientpositive/authorization_admin_almighty1.q 45c4a7d 
>   ql/src/test/queries/clientpositive/authorization_admin_almighty2.q ce99670 
>   ql/src/test/queries/clientpositive/authorization_create_func1.q 65a7b33 
>   ql/src/test/queries/clientpositive/authorization_create_macro1.q fb60500 
>   ql/src/test/queries/clientpositive/authorization_insert.q 6cce469 
>   ql/src/test/queries/clientpositive/authorization_owner_actions_db.q 36ab260 
>   ql/src/test/queries/clientpositive/authorization_role_grant1.q c062ef2 
>   ql/src/test/queries/clientpositive/authorization_role_grant2.q 34e19a2 
>   ql/src/test/queries/clientpositive/authorization_set_show_current_role.q 6b5af6e 
>   ql/src/test/queries/clientpositive/authorization_show_grant.q 5f7a33b 
>   ql/src/test/queries/clientpositive/authorization_view_sqlstd.q b04f6d6 
> 
> Diff: https://reviews.apache.org/r/24043/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>


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