hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejas Nair" <the...@hortonworks.com>
Subject Re: Review Request 17441: Adds concept of root role in Hive.
Date Tue, 28 Jan 2014 03:07:27 GMT

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



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/17441/#comment61984>

    we should add this to conf/hive-default.xml.template and document it there.
    Can you also put the same into the release notes section of jira ?
    



itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestEmbeddedHiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61986>

    can you also test if the users from config are getting added to the role ?
    



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61985>

    use ALL CAPS for final string variable name ?
    Should role name also be all CAPS ? The PUBLIC role is usually mentioned in CAPS, since
this is a similar special role, using cap letters might make sense.
    
    



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61978>

    you can make this final as well, as the object reference should not change



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61976>

    Can you fix the indentation ?



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61979>

    how about logging this in info or warn ? As this borders on an error condition.
    



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61980>

    LOG.warn would be better 



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61981>

    it would be useful to also print the value that this code saw (userStr), it might be useful
while debugging.
    Also, LOG.warn or error might be more appropriate, as the user has given an invalid value.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61982>

    LOG.error here?
    



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/17441/#comment61983>

    LOG.error


- Thejas Nair


On Jan. 28, 2014, 2:03 a.m., Ashutosh Chauhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17441/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2014, 2:03 a.m.)
> 
> 
> Review request for hive and Thejas Nair.
> 
> 
> Bugs: HIVE-5959
>     https://issues.apache.org/jira/browse/HIVE-5959
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Adds concept of root role in Hive.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 371cb0f 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestEmbeddedHiveMetaStore.java
b6b0e6e 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 58f9957 
> 
> Diff: https://reviews.apache.org/r/17441/diff/
> 
> 
> Testing
> -------
> 
> New junit test added.
> 
> 
> Thanks,
> 
> Ashutosh Chauhan
> 
>


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