You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arjun Mishra via Review Board <no...@reviews.apache.org> on 2018/01/04 21:37:44 UTC
Re: 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/#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. 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 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
>
>