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