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




bin/config_tool
Line 55 (original), 55 (patched)
<https://reviews.apache.org/r/64259/#comment272636>

    Please verify that this works.



bin/run_sentry.sh
Line 23 (original), 23 (patched)
<https://reviews.apache.org/r/64259/#comment272637>

    Does this work?



sentry-command-line-tools/pom.xml
Lines 47 (patched)
<https://reviews.apache.org/r/64259/#comment272640>

    You may consider using this instead of the two log4j dependencies. You 
should provide correct version though.
    
            <!-- https://mvnrepository.com/artifact/org.slf4j/slf4j-log4j12 -->
            <dependency>
                <groupId>org.slf4j</groupId>
                <artifactId>slf4j-log4j12</artifactId>
                <version>1.8.0-beta0</version>
            </dependency>



sentry-command-line-tools/pom.xml
Lines 60 (patched)
<https://reviews.apache.org/r/64259/#comment272641>

    I'm curios, what uses this?



sentry-command-line-tools/pom.xml
Lines 62 (patched)
<https://reviews.apache.org/r/64259/#comment272642>

    I noticed that there is 1.5.4 version of commons-pool which you get from 
hive-metastore and 2.4.2 that you get directly. Are there any potential issues 
with this?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 18 (original), 18 (patched)
<https://reviews.apache.org/r/64259/#comment272644>

    You moved the class and now package name doesn't correspond to the path - I 
am not sure whether this can cause any problems. What do you think?
    
    May be you can create package org.apache.sentry here?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 45 (original), 49 (patched)
<https://reviews.apache.org/r/64259/#comment272646>

    Do you actually use these classes now? I think you only check for valid 
command string which you do in the switch() statement below, so I guess this 
can be removed now.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 79 (original), 83 (patched)
<https://reviews.apache.org/r/64259/#comment272652>

    Since you are touching this, can you put 
    `"log4j.category.DataNucleus.Query"` in constant?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 98 (original), 102 (patched)
<https://reviews.apache.org/r/64259/#comment272648>

    It would be better to print explicit message telling that command name is 
missing if commandName is null



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 106 (original), 110 (patched)
<https://reviews.apache.org/r/64259/#comment272649>

    I don't think you need this now.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 110 (original), 114 (patched)
<https://reviews.apache.org/r/64259/#comment272645>

    It is Command, not Object



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Lines 126 (patched)
<https://reviews.apache.org/r/64259/#comment272654>

    Add `break`


- Alexander Kolbasov


On Dec. 11, 2017, 8:03 p.m., Xinran Tinney wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2017, 8:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> TheSentryMain class currently works by mapping the command name to a Java 
> class that is then dynamically loaded:
>     String commandName = commandLine.getOptionValue(COMMAND);
>     String commandClazz = COMMANDS.get(commandName);
>     Object command;
>     try {
>       command = Class.forName(commandClazz.trim()).newInstance();
>     } catch (Exception e) {
>       String msg = "Could not create instance of " + commandClazz + " for 
> command " + commandName;
>       throw new IllegalStateException(msg, e);
>     }
>     if (!(command instanceof Command)) {
>       String msg = "Command " + command.getClass().getName() + " is not an 
> instance of "
>           + Command.class.getName();
>       throw new IllegalStateException(msg);
>     }
>     ((Command)command).run(commandLine.getArgs());
>   }
> This ia too complicated and causes subtle problems at runtime. Instead it 
> should just create a new instance of appropriate class and call it directly.
> 
> 
> Diffs
> -----
> 
>   bin/config_tool 4da85673 
>   bin/run_sentry.sh d58d5e5c 
>   bin/sentry 54e545aa 
>   pom.xml dd408d85 
>   sentry-command-line-tools/pom.xml PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  3a981b2a 
> 
> 
> Diff: https://reviews.apache.org/r/64259/diff/2/
> 
> 
> Testing
> -------
> 
> mvn clean install, on cloudcat and all SUCCESS
> 
> 
> Thanks,
> 
> Xinran Tinney
> 
>

Reply via email to