> On Jan. 9, 2018, 10:16 p.m., Sergio Pena wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java > > Line 18 (original), 18 (patched) > > <https://reviews.apache.org/r/64259/diff/4/?file=1932500#file1932500line18> > > > > This package name does not corresponds to where the file is located. It > > means that SentryMain should be located in > > org/apache/sentry/shell/SentryMain.java > > > > Why is this changed needed? Is there a way to prevent this change?
The location needs to be changed since org.apache.sentry.binding.hive which is needed in SentryMain.java generates a loop of dependency and since org.apache.sentry.shell has files related to command line, I moved there. I do not know why the name is still sentry-core, I have to verify and change it. > On Jan. 9, 2018, 10:16 p.m., Sergio Pena wrote: > > sentry-tools/pom.xml > > Lines 42 (patched) > > <https://reviews.apache.org/r/64259/diff/4/?file=1932501#file1932501line42> > > > > sentry-tools/pom.xml is including a hard-coded version of > > slf4j-log4j12. We need to use a variable for it. Perhaps ${slf4j.version}? > > or ${log4j.version}? I think we do not need the version number since in the parent pom, it already has it, and this pom can depend on it. - Xinran ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64259/#review195083 ----------------------------------------------------------- On Jan. 5, 2018, 3:08 p.m., Xinran Tinney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64259/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2018, 3:08 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 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java > 3a981b2a > sentry-tools/pom.xml 45cfdb56 > > > Diff: https://reviews.apache.org/r/64259/diff/4/ > > > Testing > ------- > > mvn clean install, on cloudcat and all SUCCESS > > > Thanks, > > Xinran Tinney > >
