Review Request 69201: SENTRY-2436 Add annotations for classes that are used in binding as public
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69201/ --- Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena. Repository: sentry Description --- Sentry bindings are using some of the classes in sentry.core and common. These classes are annotated as public Diffs - sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ActiveRoleSet.java c24a6cde sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/Subject.java 88457c0d sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java e36b09a1 sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAction.java c5842d98 sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java 4ce01b2c sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Database.java e8dc1406 sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Server.java 41693c25 sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Table.java 5a981588 sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java aecfe5b5 sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java 761fb527 Diff: https://reviews.apache.org/r/69201/diff/1/ Testing --- mvn clean install Thanks, Xinran Tinney
Re: Review Request 67975: Removed unused parameters in the documentation of SqoopAuthBinding.java and SolrAuthBinding.java
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67975/ --- (Updated July 19, 2018, 7:31 p.m.) Review request for sentry and Sergio Pena. Changes --- Added the parameter "privilege" Repository: sentry Description --- Error in the documentations for SqoopAuthBinding and SolrAuthBinding Diffs (updated) - sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java 32a1fc15 sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java 539ccc13 Diff: https://reviews.apache.org/r/67975/diff/2/ Changes: https://reviews.apache.org/r/67975/diff/1-2/ Testing --- Thanks, Xinran Tinney
Review Request 67975: Removed unused parameters in the documentation of SqoopAuthBinding.java and SolrAuthBinding.java
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67975/ --- Review request for sentry and Sergio Pena. Repository: sentry Description --- Error in the documentations for SqoopAuthBinding and SolrAuthBinding Diffs - sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java 32a1fc15 sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java 539ccc13 Diff: https://reviews.apache.org/r/67975/diff/1/ Testing --- Thanks, Xinran Tinney
Review Request 66636: SENTRY-2200 Update Sentry datanucleus config names
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66636/ --- Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and Xinran Tinney. Repository: sentry Description --- Since 2.0, the datanucleus version in Sentry changed from 3.2 to 4.1. Based on the datanucleus documentation following config names are renamed. Sentry has several places use those configuration names. We should update the config names to reflect the correct names. Migration from 3.3.7 to 4.0.0.M1 Migrating will require the following changes Persistence property datanucleus.allowAttachOfTransient now defaults to true for JPA usage; set it explicitly to get old behaviour Persistence property datanucleus.metadata.validate was removed (replaced by datanucleus.metadata.xml.validate some time back) Persistence property datanucleus.defaultInheritanceStrategy is renamed to datanucleus.metadata.defaultInheritanceStrategy Persistence property datanucleus.autoCreateSchema is renamed to datanucleus.schema.autoCreateAll Persistence property datanucleus.autoCreateTables is renamed to datanucleus.schema.autoCreateTables Persistence property datanucleus.autoCreateColumns is renamed to datanucleus.schema.autoCreateColumns Persistence property datanucleus.autoCreateConstraints is renamed to datanucleus.schema.autoCreateConstraints Persistence property datanucleus.validateSchema is renamed to datanucleus.schema.validateAll Persistence property datanucleus.validateTables is renamed to datanucleus.schema.validateTables Persistence property datanucleus.validateColumns is renamed to datanucleus.schema.validateColumns Persistence property datanucleus.validateConstraints is renamed to datanucleus.schema.validateConstraints Persistence property datanucleus.fixedDatastore is now removed, since it only equated to setting the "autoCreate" properties to false. Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 83c0fc47 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 7e02874b sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java 61a74c3f sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 9b0aeb2f Diff: https://reviews.apache.org/r/66636/diff/1/ Testing --- mvn clean install all SUCCESS Thanks, Xinran Tinney
Re: Review Request 66337: SENTRY-2167: Fix the logs in NotificationProcessor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66337/#review201211 --- Ship it! Ship It! - Xinran Tinney On March 28, 2018, 4:16 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66337/ > --- > > (Updated March 28, 2018, 4:16 p.m.) > > > Review request for sentry, Na Li and Sergio Pena. > > > Bugs: SENTRY-2167 > https://issues.apache.org/jira/browse/SENTRY-2167 > > > Repository: sentry > > > Description > --- > > 1. When the location and name of the object is not changed, sentry ignores > that event. It is not an error. This should be logged with debug level. > > 2. When ever, SentryNoSuchObjectException is thrown, it is logged with INFO > level. It is common that sentry doesn't have permissions on the objects that > are changed. It need not be logged with INFO, instead it should be logged > with debug level. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java > e5ad3b5df5f1fdacd91b18f29394ec8275bb31e1 > > > Diff: https://reviews.apache.org/r/66337/diff/1/ > > > Testing > --- > > Made sure all the tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 66480: SENTRY-2140: Static Attribute Ingestion
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66480/#review201210 --- Is SENTRY-2140 titled: Attribute based access control? - Xinran Tinney On April 5, 2018, 9:20 p.m., Liam Sargent wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66480/ > --- > > (Updated April 5, 2018, 9:20 p.m.) > > > Review request for sentry, Na Li, Steve Moist, and Sergio Pena. > > > Bugs: SENTRY-2189 > https://issues.apache.org/jira/browse/SENTRY-2189 > > > Repository: sentry > > > Description > --- > > - Example configuration file: example-definition.json > - Attribute: plain old java object for string attribute. > - Object: plain old java object for fully qualified sentry object. > - AttributeMap: Bidirectional map type for quick lookup between Attribute -> > Object, Object -> Attribute. > - AttributeMapAdapter: JSON deserializer for file-based attribute ingestion. > > > Diffs > - > > pom.xml 61e0f970017aa8ddf38a59c7c1334fadb97abe40 > sentry-abac/example-definition.json PRE-CREATION > sentry-abac/example-delta.json PRE-CREATION > sentry-abac/notes.txt PRE-CREATION > sentry-abac/pom.xml PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/Attribute.java > PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMap.java > PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMapAdapter.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMapKeyException.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/GenericAttributeProvider.java > PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/SentryObject.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/StaticAttributeProvider.java > PRE-CREATION > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestAttribute.java > PRE-CREATION > > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestAttributeMap.java > PRE-CREATION > > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestStaticProvider.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66480/diff/1/ > > > Testing > --- > > - Unittests for all useful functions. > > > Thanks, > > Liam Sargent > >
Re: Review Request 66590: SENTRY-2201 Initial Attriubte based access control
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66590/#review201207 --- sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMap.java Lines 117 (patched) <https://reviews.apache.org/r/66590/#comment282338> targetObjects.size()=0 might because of line 111, so logically, there were targets for the attribute actually. sentry-abac/src/main/java/org/apache/sentry/abac/SentryAttributeAuthorizer.java Lines 146 (patched) <https://reviews.apache.org/r/66590/#comment282339> do we have coding style restrictions for long lines? - Xinran Tinney On April 12, 2018, 8:45 p.m., Steve Moist wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66590/ > --- > > (Updated April 12, 2018, 8:45 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > --- > > This is the inital draft of attribute based access control. > > > Diffs > - > > pom.xml 16a3838a > sentry-abac/example-definition.json PRE-CREATION > sentry-abac/example-delta.json PRE-CREATION > sentry-abac/notes.txt PRE-CREATION > sentry-abac/pom.xml PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/Attribute.java > PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMap.java > PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMapAdapter.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/AttributeMapKeyException.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/GenericAttributeProvider.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/SentryAttributeAuthorizer.java > PRE-CREATION > sentry-abac/src/main/java/org/apache/sentry/abac/SentryObject.java > PRE-CREATION > > sentry-abac/src/main/java/org/apache/sentry/abac/StaticAttributeProvider.java > PRE-CREATION > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestAttribute.java > PRE-CREATION > > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestAttributeMap.java > PRE-CREATION > > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestSentryAttributeAuthorizer.java > PRE-CREATION > > sentry-abac/src/test/java/org/apache/sentry/tests/abac/TestStaticProvider.java > PRE-CREATION > sentry-abac/src/test/resources/abac.props PRE-CREATION > sentry-binding/sentry-binding-hive/pom.xml ccfa9cfe > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java > 1ab5be35 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryHiveAuthorizerImpl.java > 86ff0cc2 > > > Diff: https://reviews.apache.org/r/66590/diff/3/ > > > Testing > --- > > full build,added unit tests, tested code on a cluster. > > > Thanks, > > Steve Moist > >
Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA
> On Feb. 21, 2018, 6:16 p.m., Sergio Pena wrote: > > Why is the TestHDFSIntegration.java deleted? I see it has other tests in > > that class. I am not sure, if we still need it, I will resubmit the patch. - Xinran --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65715/#review197973 --- On Feb. 20, 2018, 8:37 p.m., Xinran Tinney wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65715/ > --- > > (Updated Feb. 20, 2018, 8:37 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar > kalvagadda, Liam Sargent, Na Li, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it > or remove if it is no longer applicable. > The TestHDFSIntegration test is disabled as well. > > > Diffs > - > > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 8852cbc0 > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java > 92c0693e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java > 96a2f901 > > > Diff: https://reviews.apache.org/r/65715/diff/1/ > > > Testing > --- > > mvn clean install all success > > > Thanks, > > Xinran Tinney > >
Re: Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA
> On Feb. 21, 2018, 7:11 p.m., kalyan kumar kalvagadda wrote: > > Xinren, > > > > > > Can you hold on on this patch. I had a patch before which extends the HA > > functionality to sentry tests. That patch was not commiited because of test > > failures. > > Instead of deleteting it I would prefer fixing it to test HA. Sure, if we need to keep it, I will modify. - Xinran --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65715/#review198001 ------- On Feb. 20, 2018, 8:37 p.m., Xinran Tinney wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65715/ > --- > > (Updated Feb. 20, 2018, 8:37 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar > kalvagadda, Liam Sargent, Na Li, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it > or remove if it is no longer applicable. > The TestHDFSIntegration test is disabled as well. > > > Diffs > - > > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 8852cbc0 > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java > 92c0693e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java > 96a2f901 > > > Diff: https://reviews.apache.org/r/65715/diff/1/ > > > Testing > --- > > mvn clean install all success > > > Thanks, > > Xinran Tinney > >
Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197807
---
Ship it!
Ship It!
- Xinran Tinney
On Feb. 20, 2018, 8:04 p.m., Na Li wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
>
> (Updated Feb. 20, 2018, 8:04 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio
> Pena.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> sentry CreateTime is the difference, measured in milliseconds, between the
> current time and midnight, January 1, 1970 UTC. hive granttime is in seconds.
> So need to convert the time.
>
> The original code just cost the timestamp from long to int without converting
> milliseconds to seconds. Therefore, the timestamp value is wrong when
> retrieving the privilege from hive.
>
> The solution is to convert the time correctly.
>
>
> Diffs
> -
>
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
> b536283
>
>
> Diff: https://reviews.apache.org/r/65642/diff/3/
>
>
> Testing
> ---
>
> tested manually by stepping into code. I cannot test it in unit test because
> Hive returns timestamp as -1L for test mode
>
> DDLTask.writeGrantInfo
> static String writeGrantInfo(List privileges, boolean
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
> StringBuilder builder = new StringBuilder();
> Collections.sort(privileges, new Comparator() {
> public int compare(HivePrivilegeInfo o1, HivePrivilegeInfo o2) {
> int compare = o1.getObject().compareTo(o2.getObject());
> if (compare == 0) {
> compare = o1.getPrincipal().compareTo(o2.getPrincipal());
> }
>
> if (compare == 0) {
> compare = o1.getPrivilege().compareTo(o2.getPrivilege());
> }
>
> return compare;
> }
> });
> Iterator var3 = privileges.iterator();
>
> while(var3.hasNext()) {
> HivePrivilegeInfo privilege = (HivePrivilegeInfo)var3.next();
> HivePrincipal principal = privilege.getPrincipal();
> HivePrivilegeObject resource = privilege.getObject();
> HivePrincipal grantor = privilege.getGrantorPrincipal();
> appendNonNull(builder, resource.getDbname(), true);
> appendNonNull(builder, resource.getObjectName());
> appendNonNull(builder, resource.getPartKeys());
> appendNonNull(builder, resource.getColumns());
> appendNonNull(builder, principal.getName());
> appendNonNull(builder, principal.getType());
> appendNonNull(builder, privilege.getPrivilege().getName());
> appendNonNull(builder, privilege.isGrantOption());
> appendNonNull(builder, testMode ? -1L :
> (long)privilege.getGrantTime() * 1000L); <-- in test mode, does not
> return real timestamp
> appendNonNull(builder, grantor.getName());
> }
>
> return builder.toString();
> } else {
> return "";
> }
> }
>
>
> Thanks,
>
> Na Li
>
>
Review Request 65715: SENTRY-1720: Re-enable or remove TestHDFSIntegrationWithHA
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65715/ --- Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar kalvagadda, Na Li, and Sergio Pena. Repository: sentry Description --- The TestHDFSIntegrationWithHA test is disabled now. We need to either fix it or remove if it is no longer applicable. The TestHDFSIntegration test is disabled as well. Diffs - sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 8852cbc0 sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java 92c0693e sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java 96a2f901 Diff: https://reviews.apache.org/r/65715/diff/1/ Testing --- mvn clean install all success Thanks, Xinran Tinney
Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197787
---
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 228-229 (patched)
<https://reviews.apache.org/r/65642/#comment278068>
This is converting from long to int, will there be overflow?
- Xinran Tinney
On Feb. 17, 2018, 12:30 a.m., Na Li wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
>
> (Updated Feb. 17, 2018, 12:30 a.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio
> Pena.
>
>
> Repository: sentry
>
>
> Description
> ---
>
> sentry CreateTime is the difference, measured in milliseconds, between the
> current time and midnight, January 1, 1970 UTC. hive granttime is in seconds.
> So need to convert the time.
>
> The original code just cost the timestamp from long to int without converting
> milliseconds to seconds. Therefore, the timestamp value is wrong when
> retrieving the privilege from hive.
>
> The solution is to convert the time correctly.
>
>
> Diffs
> -
>
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
> b536283
>
>
> Diff: https://reviews.apache.org/r/65642/diff/2/
>
>
> Testing
> ---
>
> tested manually by stepping into code. I cannot test it in unit test because
> Hive returns timestamp as -1L for test mode
>
> DDLTask.writeGrantInfo
> static String writeGrantInfo(List privileges, boolean
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
> StringBuilder builder = new StringBuilder();
> Collections.sort(privileges, new Comparator() {
> public int compare(HivePrivilegeInfo o1, HivePrivilegeInfo o2) {
> int compare = o1.getObject().compareTo(o2.getObject());
> if (compare == 0) {
> compare = o1.getPrincipal().compareTo(o2.getPrincipal());
> }
>
> if (compare == 0) {
> compare = o1.getPrivilege().compareTo(o2.getPrivilege());
> }
>
> return compare;
> }
> });
> Iterator var3 = privileges.iterator();
>
> while(var3.hasNext()) {
> HivePrivilegeInfo privilege = (HivePrivilegeInfo)var3.next();
> HivePrincipal principal = privilege.getPrincipal();
> HivePrivilegeObject resource = privilege.getObject();
> HivePrincipal grantor = privilege.getGrantorPrincipal();
> appendNonNull(builder, resource.getDbname(), true);
> appendNonNull(builder, resource.getObjectName());
> appendNonNull(builder, resource.getPartKeys());
> appendNonNull(builder, resource.getColumns());
> appendNonNull(builder, principal.getName());
> appendNonNull(builder, principal.getType());
> appendNonNull(builder, privilege.getPrivilege().getName());
> appendNonNull(builder, privilege.isGrantOption());
> appendNonNull(builder, testMode ? -1L :
> (long)privilege.getGrantTime() * 1000L); <-- in test mode, does not
> return real timestamp
> appendNonNull(builder, grantor.getName());
> }
>
> return builder.toString();
> } else {
> return "";
> }
> }
>
>
> Thanks,
>
> Na Li
>
>
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 cre
Re: Review Request 65533: SENTRY-2115: Incorrect behavior of HMsFollower when HDFSSync feature is disabled.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65533/#review197329 --- sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java Lines 451 (patched) <https://reviews.apache.org/r/65533/#comment277469> 2 empty lines sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java Lines 487 (patched) <https://reviews.apache.org/r/65533/#comment277470> can this string be created as a string variable so no need to hard coded - Xinran Tinney On Feb. 6, 2018, 7:06 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65533/ > --- > > (Updated Feb. 6, 2018, 7:06 p.m.) > > > Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2115 > https://issues.apache.org/jira/browse/SENTRY-2115 > > > Repository: sentry > > > Description > --- > > **Scenario-1:** When HDFS sync is disabled, and sentry is started for the > first time. > **Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is > less than last event-d processed by sentry > **Scenario-3:** When HDFS sync is disabled, and first event-id in the > subsequent pull is not greater than the last event-id processed by sentry by > 1. > **New Behavior:** Full snapshots need not be taken in all > When Sentry detects out-of-sync situations, it should reset > SENTRY_HMS_NOTIFICATION_ID table and start processing the event in > HMS_NOTIFICATION_LOG from beginning. > > **Scenario-4:** Initially HDFS sync was enabled and later disabled for while > and then HDFS sync is enabled and sentry service is restarted to get it to > effect. > **New Behavior:** When Sentry detects out-of-sync situations, it should reset > SENTRY_HMS_NOTIFICATION_ID table and start processing the event in > HMS_NOTIFICATION_LOG from beginning. > To handle scenario explained in Scenario-4, sentry should reset the mapping > information when ever HDFS sync is disabled. That way it can learn from > scratch when the feature is enabled back. There is no value is holding stale > data even when we know it will have issues when the feature is enabled back. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > 2f2b98412e7dfdcc847ffe7975a70f452554e747 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > 79030780c35e5bda432e3ec3f01328e627cb50a6 > > > Diff: https://reviews.apache.org/r/65533/diff/1/ > > > Testing > --- > > Made sure that all the tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry
> On Feb. 9, 2018, 9:17 p.m., Xinran Tinney wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 3423 (patched) > > <https://reviews.apache.org/r/65268/diff/7/?file=1954006#file1954006line3423> > > > > "long than" "longer than" instead of "longer that" - Xinran --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65268/#review197182 --- On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65268/ > --- > > (Updated Feb. 7, 2018, 6:04 p.m.) > > > Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim > Spector. > > > Bugs: SENTRY-1904 > https://issues.apache.org/jira/browse/SENTRY-1904 > > > Repository: sentry > > > Description > --- > > The TransactionManager uses exponential backoff strategy for transaction > retries. This may cause some transactions to be delayed by a very long time. > We should also have a constraint on the max time for a transaction so that we > do not retry for too long. > > New patch that is attached adds upper bounds on below > > 1.Interval between the retry attempts which increases exponentially. > 2.Total time a transaction could spend in retries. > 3.Removed retry based on count. > > > With out these limits we would not have a control on how long a transaction > could be be active. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java > f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 7e02874b4be6a7109108809b1f404fe971b7b8e2 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > 91c90f9d302f4feb3a8b3d06541f43541c87bf0f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > b4100278392986c161625a366212c6fef66ec0a9 > > > Diff: https://reviews.apache.org/r/65268/diff/7/ > > > Testing > --- > > Made sure all the tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65268/#review197182 --- sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Lines 3423 (patched) <https://reviews.apache.org/r/65268/#comment277301> "long than" sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Lines 3461 (patched) <https://reviews.apache.org/r/65268/#comment277316> should be a new line from "if" - Xinran Tinney On Feb. 7, 2018, 6:04 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65268/ > --- > > (Updated Feb. 7, 2018, 6:04 p.m.) > > > Review request for sentry, Arjun Mishra, Na Li, Sergio Pena, and Vadim > Spector. > > > Bugs: SENTRY-1904 > https://issues.apache.org/jira/browse/SENTRY-1904 > > > Repository: sentry > > > Description > --- > > The TransactionManager uses exponential backoff strategy for transaction > retries. This may cause some transactions to be delayed by a very long time. > We should also have a constraint on the max time for a transaction so that we > do not retry for too long. > > New patch that is attached adds upper bounds on below > > 1.Interval between the retry attempts which increases exponentially. > 2.Total time a transaction could spend in retries. > 3.Removed retry based on count. > > > With out these limits we would not have a control on how long a transaction > could be be active. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java > f4ff962a67f8a5c23cc5c8daa7bcb861d2e6b6a5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 7e02874b4be6a7109108809b1f404fe971b7b8e2 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > 91c90f9d302f4feb3a8b3d06541f43541c87bf0f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > b4100278392986c161625a366212c6fef66ec0a9 > > > Diff: https://reviews.apache.org/r/65268/diff/7/ > > > Testing > --- > > Made sure all the tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
Re: Review Request 65487: Bump com.codahale.metrics package to io.dropwizard.metrics version 3.2.2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65487/#review196917 --- Ship it! Ship It! - Xinran Tinney On Feb. 2, 2018, 7:31 p.m., Liam Sargent wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65487/ > --- > > (Updated Feb. 2, 2018, 7:31 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Steve > Moist, and Sergio Pena. > > > Bugs: SENTRY-2136 > https://issues.apache.org/jira/browse/SENTRY-2136 > > > Repository: sentry > > > Description > --- > > Bump com.codahale.metrics package to io.dropwizard.metrics version 3.2.2 > > > Diffs > - > > pom.xml 6f9856e45b72ef9e0c43a222eddc8452b64f1a71 > sentry-provider/sentry-provider-db/pom.xml > 5733445af481fd83bb71189178647af234fe77a1 > sentry-tests/sentry-tests-solr/pom.xml > 5ef7a2b1de67a2f35510ad41c0150ad1bc957118 > > > Diff: https://reviews.apache.org/r/65487/diff/1/ > > > Testing > --- > > mvn test - ALL PASS > > > Thanks, > > Liam Sargent > >
Review Request 65388: SENTRY-2124 LeaderStatusMonitor.toString() throws IllegalFormatConversionException with AtomicLong
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65388/ --- Review request for sentry, Alexander Kolbasov, Arjun Mishra, Colm O hEigeartaigh, kalyan kumar kalvagadda, Na Li, and Sergio Pena. Repository: sentry Description --- When I start Sentry, I noticed an exception on the console due to an illegal format string in LeaderStatusMonitor.toString() java.util.IllegalFormatConversionException: d != java.util.concurrent.atomic.AtomicLong at java.util.Formatter$FormatSpecifier.failConversion(Formatter.java:4302) at java.util.Formatter$FormatSpecifier.printInteger(Formatter.java:2793) at java.util.Formatter$FormatSpecifier.print(Formatter.java:2747) at java.util.Formatter.format(Formatter.java:2520) at java.util.Formatter.format(Formatter.java:2455) at java.lang.String.format(String.java:2940) at org.apache.sentry.provider.db.service.persistent.LeaderStatusMonitor.toString(LeaderStatusMonitor.java:282) at org.slf4j.helpers.MessageFormatter.safeObjectAppend(MessageFormatter.java:297) at org.slf4j.helpers.MessageFormatter.deeplyAppendParameter(MessageFormatter.java:269) at org.slf4j.helpers.MessageFormatter.arrayFormat(MessageFormatter.java:227) at org.slf4j.helpers.MessageFormatter.format(MessageFormatter.java:124) at org.slf4j.impl.Log4jLoggerAdapter.info(Log4jLoggerAdapter.java:322) at org.apache.sentry.provider.db.service.persistent.LeaderStatusMonitor.takeLeadership(LeaderStatusMonitor.java:251) at sentry.org.apache.curator.framework.recipes.leader.LeaderSelector$WrappedListener.takeLeadership(LeaderSelector.java:537) at sentry.org.apache.curator.framework.recipes.leader.LeaderSelector.doWork(LeaderSelector.java:399) at sentry.org.apache.curator.framework.recipes.leader.LeaderSelector.doWorkLoop(LeaderSelector.java:444) at sentry.org.apache.curator.framework.recipes.leader.LeaderSelector.access$100(LeaderSelector.java:64) at sentry.org.apache.curator.framework.recipes.leader.LeaderSelector$2.call(LeaderSelector.java:245) at sentry.org.apache.curator.framework.recipes.leader.LeaderSelector$2.call(LeaderSelector.java:239) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:748) Diffs - sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LeaderStatusMonitor.java 25a70bda Diff: https://reviews.apache.org/r/65388/diff/1/ Testing --- mvn clean install Thanks, Xinran Tinney
Re: Review Request 65046: SENTRY-1819 HMSFollower and friends do not belong in sentry.service.thrift
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65046/ --- (Updated Jan. 11, 2018, 10:14 p.m.) Review request for sentry, Alexander Kolbasov, Arjun Mishra, Colm O hEigeartaigh, kalyan kumar kalvagadda, Na Li, and Sergio Pena. Changes --- Added all the actual imports instead of using .*; Repository: sentry Description --- We have several important classes - e.g. HMSFollower, NotificationProcessor, CounterWait, LeaderStatusMonitor in the sentry.service.thrift package which is weird - they should be in provider.db.service.persistent instead. Diffs (updated) - sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java eee44892 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631fa sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java 558e6953 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java 86ff47e0 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 097aa629 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 77634cf2 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java 8e80d558 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java d09da5fb sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java 7e774b4a sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 43535a7b sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 501898bc sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java 090999a0 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatusMonitor.java 72d52de1 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java 964a56cd Diff: https://reviews.apache.org/r/65046/diff/4/ Changes: https://reviews.apache.org/r/65046/diff/3-4/ Testing --- 'mvn clean install' on testing cluster. Thanks, Xinran Tinney
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)
> > <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 65046: SENTRY-1819 HMSFollower and friends do not belong in sentry.service.thrift
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65046/ --- (Updated Jan. 9, 2018, 9:45 p.m.) Review request for sentry, Alexander Kolbasov, Arjun Mishra, Colm O hEigeartaigh, kalyan kumar kalvagadda, Na Li, and Sergio Pena. Changes --- Sync with SENTRY-2085 Repository: sentry Description --- We have several important classes - e.g. HMSFollower, NotificationProcessor, CounterWait, LeaderStatusMonitor in the sentry.service.thrift package which is weird - they should be in provider.db.service.persistent instead. Diffs (updated) - sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java eee44892 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631fa sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java 558e6953 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java 86ff47e0 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 097aa629 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java 77634cf2 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java 8e80d558 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java d09da5fb sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java 7e774b4a sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 43535a7b sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java 501898bc sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java 090999a0 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatusMonitor.java 72d52de1 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java 964a56cd Diff: https://reviews.apache.org/r/65046/diff/3/ Changes: https://reviews.apache.org/r/65046/diff/2-3/ Testing --- 'mvn clean install' on testing cluster. Thanks, Xinran Tinney
Review Request 65046: SENTRY-1819 HMSFollower and friends do not belong in sentry.service.thrift
/service/persistent/TestNotificationProcessor.java PRE-CREATION sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java 090999a sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886 sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatusMonitor.java 72d52de sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java 964a56c sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java 4430ce7 sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestLocalGroupMapping.java 40cb814 sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java 9da3d6e sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56 sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java fd8ec56 Diff: https://reviews.apache.org/r/65046/diff/1/ Testing --- 'mvn clean install' on testing cluster. Thanks, Xinran Tinney
Review Request 65028: SENTRY-1819 HMSFollower and friends do not belong in sentry.service.thrift
/TestUserManagement.java fd8ec56a sentry-tools/src/main/java/org/apache/sentry/shell/SentryMain.java PRE-CREATION Diff: https://reviews.apache.org/r/65028/diff/1/ Testing --- 'mvn clean install' on the cluster and all tests succeess. Thanks, Xinran Tinney
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, 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
> 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 64890: SENTRY-1165 add clover plugin to maven to get code coverage report
> On Jan. 2, 2018, 5:04 p.m., Steve Moist wrote: > > pom.xml > > Lines 117 (patched) > > <https://reviews.apache.org/r/64890/diff/1/?file=1929483#file1929483line117> > > > > Looks like 4.2.1 is the latest version, any reason why we're not using > > it? > > Xinran Tinney wrote: > I was following an example that was 4.2.0, let me test 4.2.1 and if it > works fine, I will switch. I found that even without adding dependency of clover in the pom, it works as long as plugin is mentioned in maven's setting.xml. Should I delete this code review? - Xinran --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64890/#review194640 ------- On Jan. 2, 2018, 4:43 p.m., Xinran Tinney wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64890/ > --- > > (Updated Jan. 2, 2018, 4:43 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > add clover plugin to maven to get code coverage report > > > Diffs > - > > pom.xml c797181c > > > Diff: https://reviews.apache.org/r/64890/diff/1/ > > > Testing > --- > > mvn clean install on testing cluster and all the tests past. > The steps for generating a clover report is as folllows: (Since clover has > become an opensource now, the one used is openclover) > 1. Put > > org.openclover > > In maven/conf/settings.xml > 2. It is required to first create a Clover database, run > mvn clover:instrument > 3. To generate report, run > mvn clover:clover > The report is in each module's folder e.g., > .../sentry/sentry-provider/sentry-provider-db/target/site/clover/clover.xml' > > > Thanks, > > Xinran Tinney > >
Re: Review Request 64317: SENTRY-2085: Keep sentry exceptions contained within Sentry
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64317/#review194723 --- sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Line 77 (original), 79 (patched) <https://reviews.apache.org/r/64317/#comment273666> There is another parameter 'roleSet' not mentioned here - Xinran Tinney On Dec. 29, 2017, 11:30 p.m., Zachary Amsden wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64317/ > --- > > (Updated Dec. 29, 2017, 11:30 p.m.) > > > Review request for sentry and Na Li. > > > Repository: sentry > > > Description > --- > > Instead of leaking new exceptions outside the API, use the > existing authorization exceptions to indicate authorization > failure when a user has no group configured. > > > Diffs > - > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 8ce7a02 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > 9c60c22 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java > a41d1bd > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrPluginImpl.java > 91d08f0 > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > 803e5ea > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java > f060b82 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryGroupNotFoundException.java > b978df6 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java > 2d82bcf > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/GroupMappingService.java > 7e85261 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupMappingService.java > bde53d5 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 005724f > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestNoAuthorizationProvider.java > fe01b06 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 650880b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java > 6597a7c > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java > 5447420 > > sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestLocalGroupMapping.java > 9864b82 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/QueryDocAuthorizationComponent.java > 2338ab8 > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java > 02ac514 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java > 02ac514 > > > Diff: https://reviews.apache.org/r/64317/diff/4/ > > > Testing > --- > > Running JUnit tests with mvn install. > > > Thanks, > > Zachary Amsden > >
Re: Review Request 64890: SENTRY-1165 add clover plugin to maven to get code coverage report
> On Jan. 2, 2018, 5:04 p.m., Steve Moist wrote: > > pom.xml > > Lines 117 (patched) > > <https://reviews.apache.org/r/64890/diff/1/?file=1929483#file1929483line117> > > > > Looks like 4.2.1 is the latest version, any reason why we're not using > > it? I was following an example that was 4.2.0, let me test 4.2.1 and if it works fine, I will switch. - Xinran --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64890/#review194640 ------- On Jan. 2, 2018, 4:43 p.m., Xinran Tinney wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64890/ > --- > > (Updated Jan. 2, 2018, 4:43 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena. > > > Repository: sentry > > > Description > --- > > add clover plugin to maven to get code coverage report > > > Diffs > - > > pom.xml c797181c > > > Diff: https://reviews.apache.org/r/64890/diff/1/ > > > Testing > --- > > mvn clean install on testing cluster and all the tests past. > The steps for generating a clover report is as folllows: (Since clover has > become an opensource now, the one used is openclover) > 1. Put > > org.openclover > > In maven/conf/settings.xml > 2. It is required to first create a Clover database, run > mvn clover:instrument > 3. To generate report, run > mvn clover:clover > The report is in each module's folder e.g., > .../sentry/sentry-provider/sentry-provider-db/target/site/clover/clover.xml' > > > Thanks, > > Xinran Tinney > >
Review Request 64890: SENTRY-1165 add clover plugin to maven to get code coverage report
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64890/ --- Review request for sentry. Repository: sentry Description --- add clover plugin to maven to get code coverage report Diffs - pom.xml c797181c Diff: https://reviews.apache.org/r/64890/diff/1/ Testing --- mvn clean install on testing cluster and all the tests past. The steps for generating a clover report is as folllows: (Since clover has become an opensource now, the one used is openclover) 1. Put org.openclover In maven/conf/settings.xml 2. It is required to first create a Clover database, run mvn clover:instrument 3. To generate report, run mvn clover:clover The report is in each module's folder e.g., .../sentry/sentry-provider/sentry-provider-db/target/site/clover/clover.xml' Thanks, Xinran Tinney
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 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
> 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.
> >
> >
> >
> > org.slf4j
> > slf4j-log4j12
> > 1.8.0-beta0
> >
I saw that in the parent pom, log4j already has
1.2.16, 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
> 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
---
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
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 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63596/#review192298 --- sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Lines 3574-3580 (patched) <https://reviews.apache.org/r/63596/#comment270362> Since this part is repeating through multiple files, is it helpful to create a function and call the function? - Xinran Tinney On Nov. 29, 2017, 5:23 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63596/ > --- > > (Updated Nov. 29, 2017, 5:23 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na > Li, Sergio Pena, and Vadim Spector. > > > Repository: sentry > > > Description > --- > > The old retrieveFullPathsImage() method in SentryStore is no longer used by > actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used > instead. It was preserved because it is used by test which now doesn't make > much sense. > > > Diffs > - > > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java > b355630e7 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java > f09d1b228 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f32a745ed > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 24b11f724 > > > Diff: https://reviews.apache.org/r/63596/diff/5/ > > > Testing > --- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore > mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test > -Dtest=TestSentryHDFSServiceProcessor > mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test > -Dtest=TestImageRetriever.java > > > Thanks, > > Arjun Mishra > >
Re: Review Request 63668: SENTRY-2038 - Some ShellCommand improvements
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63668/#review190862 --- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/GenericShellCommand.java Lines 112 (patched) <https://reviews.apache.org/r/63668/#comment268425> Hi, can this be done through calling client.listRolesByGroupName()? - Xinran Tinney On Nov. 8, 2017, 3:34 p.m., Colm O hEigeartaigh wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63668/ > --- > > (Updated Nov. 8, 2017, 3:34 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-2038 > https://issues.apache.org/jira/browse/SENTRY-2038 > > > Repository: sentry > > > Description > --- > > This issue is for three fairly minor ShellCommand improvements: > > a) "roleName" is not required in ShellCommand.listRoles > b) Change the methods that split a "groups" String, to just take in a Set > instead. This means I can re-use them in the CLI branch. > c) Add a new "listGroupRoles" implementation, from the CLI branch. It's a > nice new command that allows you to list all groups and all roles associated > with those groups. > > > Diffs > - > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellGeneric.java > 49f18c89 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/GenericShellCommand.java > 11615ffa > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java > dd245eac > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java > 226d58d5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/ShellCommand.java > ec751ecf > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/HiveShellCommand.java > 1e0692b5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellKafka.java > 80bbcf18 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java > 55831a45 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSqoop.java > 7bafd8c4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java > adfd102c > > > Diff: https://reviews.apache.org/r/63668/diff/1/ > > > Testing > --- > > Tested the script. > > > Thanks, > > Colm O hEigeartaigh > >
