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/#review197974 --- Ship it! Thanks Xinran, then I think this change is correct as

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 hav

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? I have not, how to verify it is correct? just run .sh? - Xinran

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/#review195270 --- It looks good. Did you test that all the commands which use Sentr

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/ --- (Updated Jan. 10, 2018, 9:38 p.m.) Review request for sentry, Alexander Kolbaso

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

> 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) > > > > > > This package name does not correspo

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/#review195083 --- sentry-core/sentry-core-common/src/main/java/org/apache/sentry/Se

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/ --- (Updated Jan. 5, 2018, 3:08 p.m.) Review request for sentry, Alexander Kolbasov

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

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

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

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

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

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

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-

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/ --- (Updated Dec. 20, 2017, 4:54 p.m.) Review request for sentry, Alexander Kolbaso

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote: > > sentry-command-line-tools/pom.xml > > Lines 47 (patched) > > > > > > You may consider using this instead of the two log4j dependencies. You > > should prov

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

> 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) > > > > > > Do you actually use these cl

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

> On Dec. 15, 2017, 7:51 p.m., Alexander Kolbasov wrote: > > sentry-command-line-tools/pom.xml > > Lines 60 (patched) > > > > > > I'm curios, what uses this? I don't think we use it here, so I will remove it > On D

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/#review193957 --- bin/config_tool Line 55 (original), 55 (patched)

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/ --- (Updated Dec. 11, 2017, 8:03 p.m.) Review request for sentry, Alexander Kolbaso

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

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

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/#review192785 --- This project has a lot of command line tools, maybe rename to sen

Re: Review Request 64259: SENTRY-1572 SentryMain() shouldn't dynamically load tool class

> 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

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/#review192691 --- Why is a new "sentry-main" module needed as part of this fix? - C

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 kalvag