You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Xinran Tinney <yu...@gmail.com> on 2018/02/15 17:46:34 UTC
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
> On Jan. 11, 2018, 11:19 p.m., Sergio Pena wrote:
> > It looks good.
> >
> > Did you test that all the commands which use SentryMain work correctly?
> > bin/sentry, bin/run_sentry.sh, bin/config_tool?
>
> Xinran Tinney wrote:
> I have not, how to verify it is correct? just run .sh?
I have run "./run_sentry.sh --command schema-tool --conffile sentry-site.xml --dbType mysql --initSchema" under the patch and it shows the same result as without the patch on sentry master:
[WARNING] Couldn't destroy threadgroup org.codehaus.mojo.exec.ExecJavaMojo$IsolatedThreadGroup[name=org.apache.sentry.SentryMain,maxpri=10]
java.lang.IllegalThreadStateException
at java.lang.ThreadGroup.destroy(ThreadGroup.java:778)
at org.codehaus.mojo.exec.ExecJavaMojo.execute(ExecJavaMojo.java:321)
at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:207)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:116)
at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:80)
at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:51)
at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:307)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:193)
at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:106)
at org.apache.maven.cli.MavenCli.execute(MavenCli.java:863)
at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:288)
at org.apache.maven.cli.MavenCli.main(MavenCli.java:199)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:289)
at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:229)
at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:415)
at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:356)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 18.759 s
[INFO] Finished at: 2018-02-15T09:41:35-08:00
[INFO] Final Memory: 44M/609M
[INFO] ------------------------------------------------------------------------
- Xinran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review195270
-----------------------------------------------------------
On Jan. 10, 2018, 9:38 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2018, 9:38 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/sentry 54e545aa
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 3a981b2a
> sentry-tools/pom.xml 45cfdb56
> sentry-tools/src/main/java/org/apache/sentry/SentryMain.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64259/diff/5/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>