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 2017/12/01 21:15:04 UTC
Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically
load tool class
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/
-----------------------------------------------------------
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-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 3a981b2a
sentry-main/pom.xml PRE-CREATION
Diff: https://reviews.apache.org/r/64259/diff/1/
Testing
-------
mvn clean install, on cloudcat and all SUCCESS
Thanks,
Xinran Tinney
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 4, 2017, 1 p.m., Colm O hEigeartaigh wrote:
> > Why is a new "sentry-main" module needed as part of this fix?
because of dependency. If the command is under one of existing directories, it will create circular dependency. That may be why the original code takes that approach "mapping the command name to a Java class that is then dynamically loaded"
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review192691
-----------------------------------------------------------
On Dec. 1, 2017, 9:15 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 9:15 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-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 3a981b2a
> sentry-main/pom.xml PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64259/diff/1/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> On Dec. 4, 2017, 1 p.m., Colm O hEigeartaigh wrote:
> > Why is a new "sentry-main" module needed as part of this fix?
>
> Na Li wrote:
> because of dependency. If the command is under one of existing directories, it will create circular dependency. That may be why the original code takes that approach "mapping the command name to a Java class that is then dynamically loaded"
Because there is dependency loop if we are going to call SentryService.CommandImpl() etc, So Sasha and Lina Suggested a new module.
- Xinran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review192691
-----------------------------------------------------------
On Dec. 1, 2017, 9:15 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 9:15 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-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 3a981b2a
> sentry-main/pom.xml PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64259/diff/1/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Colm O hEigeartaigh <co...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review192691
-----------------------------------------------------------
Why is a new "sentry-main" module needed as part of this fix?
- Colm O hEigeartaigh
On Dec. 1, 2017, 9:15 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 9:15 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-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 3a981b2a
> sentry-main/pom.xml PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64259/diff/1/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Steve Moist via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review192785
-----------------------------------------------------------
This project has a lot of command line tools, maybe rename to sentry-command-line-tools or something of that effect?
pom.xml
Lines 647 (patched)
<https://reviews.apache.org/r/64259/#comment271042>
whitespace
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Lines 128 (patched)
<https://reviews.apache.org/r/64259/#comment271041>
Command won't be null as printHelp does a system.exit.
- Steve Moist
On Dec. 1, 2017, 9:15 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 9:15 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-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java 3a981b2a
> sentry-main/pom.xml PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/64259/diff/1/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Steve Moist via Review Board <no...@reviews.apache.org>.
> On Jan. 4, 2018, 9:37 p.m., Arjun Mishra wrote:
> > I don't think you meant to commit "sentry-dist/src/license/THIRD-PARTY.properties". Am I right? It is being generated because of another issue. I am not sure if you intentionally created it
I have also seen this on another review as well, I also assume it's by accident, we should do something to prevent this unless there are actual changes to the liceneses.
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review194789
-----------------------------------------------------------
On Dec. 20, 2017, 4:54 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2017, 4:54 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-dist/src/license/THIRD-PARTY.properties 22429fc3
> sentry-tools/pom.xml 45cfdb56
>
>
> Diff: https://reviews.apache.org/r/64259/diff/3/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> On Jan. 4, 2018, 9:37 p.m., Arjun Mishra wrote:
> > I don't think you meant to commit "sentry-dist/src/license/THIRD-PARTY.properties". Am I right? It is being generated because of another issue. I am not sure if you intentionally created it
>
> Steve Moist wrote:
> I have also seen this on another review as well, I also assume it's by accident, we should do something to prevent this unless there are actual changes to the liceneses.
>
> Arjun Mishra wrote:
> Kalyan has this on his plate. We should be resolving this soon. I think it was this ticket SENTRY-2081 that caused it https://github.com/apache/sentry/commit/10de29b392f7ba86e1f71867d27dda3cda958a7e
Yes, it is generated when "mvn clean install", I guess I should generate the patch first and then test it
- Xinran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review194789
-----------------------------------------------------------
On Dec. 20, 2017, 4:54 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2017, 4:54 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-dist/src/license/THIRD-PARTY.properties 22429fc3
> sentry-tools/pom.xml 45cfdb56
>
>
> Diff: https://reviews.apache.org/r/64259/diff/3/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Jan. 4, 2018, 9:37 p.m., Arjun Mishra wrote:
> > I don't think you meant to commit "sentry-dist/src/license/THIRD-PARTY.properties". Am I right? It is being generated because of another issue. I am not sure if you intentionally created it
>
> Steve Moist wrote:
> I have also seen this on another review as well, I also assume it's by accident, we should do something to prevent this unless there are actual changes to the liceneses.
Kalyan has this on his plate. We should be resolving this soon. I think it was this ticket SENTRY-2081 that caused it https://github.com/apache/sentry/commit/10de29b392f7ba86e1f71867d27dda3cda958a7e
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review194789
-----------------------------------------------------------
On Dec. 20, 2017, 4:54 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2017, 4:54 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-dist/src/license/THIRD-PARTY.properties 22429fc3
> sentry-tools/pom.xml 45cfdb56
>
>
> Diff: https://reviews.apache.org/r/64259/diff/3/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review194789
-----------------------------------------------------------
I don't think you meant to commit "sentry-dist/src/license/THIRD-PARTY.properties". Am I right? It is being generated because of another issue. I am not sure if you intentionally created it
- Arjun Mishra
On Dec. 20, 2017, 4:54 p.m., Xinran Tinney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64259/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2017, 4:54 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-dist/src/license/THIRD-PARTY.properties 22429fc3
> sentry-tools/pom.xml 45cfdb56
>
>
> Diff: https://reviews.apache.org/r/64259/diff/3/
>
>
> Testing
> -------
>
> mvn clean install, on cloudcat and all SUCCESS
>
>
> Thanks,
>
> Xinran Tinney
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> 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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review195083
-----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
Line 18 (original), 18 (patched)
<https://reviews.apache.org/r/64259/#comment274221>
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?
sentry-tools/pom.xml
Lines 42 (patched)
<https://reviews.apache.org/r/64259/#comment274220>
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}?
- Sergio Pena
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> 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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> 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?
I have not, how to verify it is correct? just run .sh?
- 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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review195270
-----------------------------------------------------------
It looks good.
Did you test that all the commands which use SentryMain work correctly?
bin/sentry, bin/run_sentry.sh, bin/config_tool?
- Sergio Pena
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review197974
-----------------------------------------------------------
Ship it!
Thanks Xinran, then I think this change is correct as the SentryMain would haven't run if the JAR location was incorrect.
- Sergio Pena
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Fixed the path of SentryMain.java
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/64259/diff/4-5/
Testing
-------
mvn clean install, on cloudcat and all SUCCESS
Thanks,
Xinran Tinney
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
removed THIRD_PARTY.properties
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/64259/diff/3-4/
Testing
-------
mvn clean install, on cloudcat and all SUCCESS
Thanks,
Xinran Tinney
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/
-----------------------------------------------------------
(Updated Dec. 20, 2017, 4:54 p.m.)
Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
Changes
-------
Fixed the issues mentioned by Sasha.
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 (updated)
-----
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-dist/src/license/THIRD-PARTY.properties 22429fc3
sentry-tools/pom.xml 45cfdb56
Diff: https://reviews.apache.org/r/64259/diff/3/
Changes: https://reviews.apache.org/r/64259/diff/2-3/
Testing
-------
mvn clean install, on cloudcat and all SUCCESS
Thanks,
Xinran Tinney
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
> > Line 45 (original), 49 (patched)
> > <https://reviews.apache.org/r/64259/diff/2/?file=1912668#file1912668line49>
> >
> > 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.
>
> Xinran Tinney wrote:
> I think it probably still going to be needed since on line 67: options.addOption(null, COMMAND, true, "Command to run. Options: " + COMMANDS.keySet());
> what do you think?
The only thing that is used is a list f commands to show - you may just have the list of valid commands to preserve this. Or just print static string - it doesn't matter much here IMO.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review193957
-----------------------------------------------------------
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote:
> > sentry-command-line-tools/pom.xml
> > Lines 60 (patched)
> > <https://reviews.apache.org/r/64259/diff/2/?file=1912667#file1912667line60>
> >
> > I'm curios, what uses this?
I don't think we use it here, so I will remove it
> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
> > Line 45 (original), 49 (patched)
> > <https://reviews.apache.org/r/64259/diff/2/?file=1912668#file1912668line49>
> >
> > 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.
I think it probably still going to be needed since on line 67: options.addOption(null, COMMAND, true, "Command to run. Options: " + COMMANDS.keySet());
what do you think?
- Xinran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review193957
-----------------------------------------------------------
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote:
> > sentry-command-line-tools/pom.xml
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/64259/diff/2/?file=1912667#file1912667line47>
> >
> > 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>
I saw that in the parent pom, log4j already has <log4j.version>1.2.16</log4j.version>, should we just ignore version in the child pom?
- Xinran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64259/#review193957
-----------------------------------------------------------
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't
dynamically load tool class
Posted by Xinran Tinney <yu...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
removed space and un-necessary check
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/64259/diff/1-2/
Testing
-------
mvn clean install, on cloudcat and all SUCCESS
Thanks,
Xinran Tinney