Review Request 63619: SENTRY-2033: Fix TestDbPrivilegeCleanupOnDrop to use SentryMetastorePostEventListenerNotificationLog

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63619/
---

Review request for sentry, Na Li and Sergio Pena.


Bugs: SENTRY-2033
https://issues.apache.org/jira/browse/SENTRY-2033


Repository: sentry


Description
---

TestDbPrivilegeCleanupOnDrop still uses SentryMetastorePostEventListener to get 
the updates on the changes in HMS data. SentryMetastorePostEventListener is 
obsolete, and the tests should not use it any more.Tests should use 
implementations of MetaStoreEventListener to process the notifications.

Here is the summary of code changes done:
1. Change the base class from AbstractTestWithStaticConfiguration to 
TestHDFSIntegrationBase. As AbstractTestWithStaticConfiguration does not 
creae/start HMS service.
2. Made code changes to update property "sentry.hive.server"
3. Code changes to TestDbPrivilegeCleanupOnDrop to be a child class for 
TestHDFSIntegrationBase.


Diffs
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 33c91244af40fabb9eca246a7664bed345e3c9f3 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 3c96d553b34d6a9fe9c9eb0a6fa88b17a4fc7694 


Diff: https://reviews.apache.org/r/63619/diff/1/


Testing
---

Made sure that the tests pass


Thanks,

kalyan kumar kalvagadda

Re: Review Request 63424: SENTRY-2024: Specify Char Set for AUTHZ_OBJ_NAME

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63424/#review190334
---



Let's not commit this yet until we understand why AUTHZ_OBJ_NAME is Binary and 
if it can be set to a simple varchar() with CHARACTER SET utf8. 
Would this work?

If it doesn't, then should we be consistent with Hive and set CHARACTER SET 
latin1 to all the varchar() columns?

- Sergio Pena


On Nov. 1, 2017, 7:32 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63424/
> ---
> 
> (Updated Nov. 1, 2017, 7:32 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_OBJ_NAME has (384) chars. It is defieved from DB_NAME and TBL_NAME from 
> hive. hive is using latin1 char set for those fields. To be consistent with 
> hive, specify this field in sentry to latin1.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  a7db1b7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> 9f61154 
> 
> 
> Diff: https://reviews.apache.org/r/63424/diff/1/
> 
> 
> Testing
> ---
> 
> tested on system
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63518: SENTRY-1997 - "Bump sqoop dependency version to 1.99.7"

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63518/#review190336
---


Ship it!




Looks good. I see only tests are modified to make it work with the new sqoop 
version.
+1

- Sergio Pena


On Nov. 2, 2017, 5:17 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63518/
> ---
> 
> (Updated Nov. 2, 2017, 5:17 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1997
> https://issues.apache.org/jira/browse/SENTRY-1997
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Here is a patch for this issue, which includes the update as well as 
> switching the tests to use the new Jetty Server instead of Tomcat. It was 
> quite tricky to resolve, I had to work around 2 issues in Sqoop which I have 
> submitted patches for:
> 
> https://issues.apache.org/jira/browse/SQOOP-3250
> https://issues.apache.org/jira/browse/SQOOP-3251
> 
> 
> Diffs
> -
> 
>   pom.xml af544809 
>   sentry-tests/sentry-tests-sqoop/pom.xml 6723c4d0 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java
>  2971bf8c 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/JettySqoopRunner.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestConnectorEndToEnd.java
>  27f14209 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestGrantPrivilege.java
>  8c7753ed 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestJobEndToEnd.java
>  636e2697 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestLinkEndToEnd.java
>  8c8a91dd 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestOwnerPrivilege.java
>  abef80c5 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestRevokePrivilege.java
>  f71595c0 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestServerScopeEndToEnd.java
>  85bae92b 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TestShowPrivilege.java
>  0ccbf5d3 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/TomcatSqoopRunner.java
>  0f0496bc 
> 
> 
> Diff: https://reviews.apache.org/r/63518/diff/1/
> 
> 
> Testing
> ---
> 
> Tested successfully with a deployment as well.
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Review Request 63619: SENTRY-2033: Fix TestDbPrivilegeCleanupOnDrop to use SentryMetastorePostEventListenerNotificationLog

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63619/#review190337
---




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
Line 44 (original), 38 (patched)


Why did it change to TestHDFSIntegrationBase if this is a Hive e2e test?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
Lines 103-105 (original), 104-113 (patched)


This code is repeated on every test. How was it working before? Why isn't 
created on a @before method?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
Lines 859 (patched)


Why was this removed?


- Sergio Pena


On Nov. 7, 2017, 4:29 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63619/
> ---
> 
> (Updated Nov. 7, 2017, 4:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2033
> https://issues.apache.org/jira/browse/SENTRY-2033
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TestDbPrivilegeCleanupOnDrop still uses SentryMetastorePostEventListener to 
> get the updates on the changes in HMS data. SentryMetastorePostEventListener 
> is obsolete, and the tests should not use it any more.Tests should use 
> implementations of MetaStoreEventListener to process the notifications.
> 
> Here is the summary of code changes done:
> 1. Change the base class from AbstractTestWithStaticConfiguration to 
> TestHDFSIntegrationBase. As AbstractTestWithStaticConfiguration does not 
> creae/start HMS service.
> 2. Made code changes to update property "sentry.hive.server"
> 3. Code changes to TestDbPrivilegeCleanupOnDrop to be a child class for 
> TestHDFSIntegrationBase.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  33c91244af40fabb9eca246a7664bed345e3c9f3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  3c96d553b34d6a9fe9c9eb0a6fa88b17a4fc7694 
> 
> 
> Diff: https://reviews.apache.org/r/63619/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that the tests pass
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 63619: SENTRY-2033: Fix TestDbPrivilegeCleanupOnDrop to use SentryMetastorePostEventListenerNotificationLog

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63619/
---

(Updated Nov. 7, 2017, 6:17 p.m.)


Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.


Changes
---

addressed review comments.


Bugs: SENTRY-2033
https://issues.apache.org/jira/browse/SENTRY-2033


Repository: sentry


Description
---

TestDbPrivilegeCleanupOnDrop still uses SentryMetastorePostEventListener to get 
the updates on the changes in HMS data. SentryMetastorePostEventListener is 
obsolete, and the tests should not use it any more.Tests should use 
implementations of MetaStoreEventListener to process the notifications.

Here is the summary of code changes done:
1. Change the base class from AbstractTestWithStaticConfiguration to 
TestHDFSIntegrationBase. As AbstractTestWithStaticConfiguration does not 
creae/start HMS service.
2. Made code changes to update property "sentry.hive.server"
3. Code changes to TestDbPrivilegeCleanupOnDrop to be a child class for 
TestHDFSIntegrationBase.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
 33c91244af40fabb9eca246a7664bed345e3c9f3 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 3c96d553b34d6a9fe9c9eb0a6fa88b17a4fc7694 


Diff: https://reviews.apache.org/r/63619/diff/2/

Changes: https://reviews.apache.org/r/63619/diff/1-2/


Testing
---

Made sure that the tests pass


Thanks,

kalyan kumar kalvagadda

Review Request 63646: SENTRY-2035: Metrics should move to destination atomically

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63646/
---

Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio 
Pena, and Vadim Spector.


Bugs: ENTRY-2035
https://issues.apache.org/jira/browse/ENTRY-2035


Repository: sentry


Description
---

SENTRY-2035: Metrics should move to destination atomically


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 86cae64fd4bbab162a09a1ca27a6d3887bfc2dc1 


Diff: https://reviews.apache.org/r/63646/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 63619: SENTRY-2033: Fix TestDbPrivilegeCleanupOnDrop to use SentryMetastorePostEventListenerNotificationLog

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63619/#review190369
---


Ship it!




Ship It!

- Sergio Pena


On Nov. 7, 2017, 6:17 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63619/
> ---
> 
> (Updated Nov. 7, 2017, 6:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2033
> https://issues.apache.org/jira/browse/SENTRY-2033
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TestDbPrivilegeCleanupOnDrop still uses SentryMetastorePostEventListener to 
> get the updates on the changes in HMS data. SentryMetastorePostEventListener 
> is obsolete, and the tests should not use it any more.Tests should use 
> implementations of MetaStoreEventListener to process the notifications.
> 
> Here is the summary of code changes done:
> 1. Change the base class from AbstractTestWithStaticConfiguration to 
> TestHDFSIntegrationBase. As AbstractTestWithStaticConfiguration does not 
> creae/start HMS service.
> 2. Made code changes to update property "sentry.hive.server"
> 3. Code changes to TestDbPrivilegeCleanupOnDrop to be a child class for 
> TestHDFSIntegrationBase.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java
>  33c91244af40fabb9eca246a7664bed345e3c9f3 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  3c96d553b34d6a9fe9c9eb0a6fa88b17a4fc7694 
> 
> 
> Diff: https://reviews.apache.org/r/63619/diff/2/
> 
> 
> Testing
> ---
> 
> Made sure that the tests pass
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 63646: SENTRY-2035: Metrics should move to destination atomically

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63646/#review190374
---


Ship it!




Ship It!

- Sergio Pena


On Nov. 7, 2017, 9:47 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63646/
> ---
> 
> (Updated Nov. 7, 2017, 9:47 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: ENTRY-2035
> https://issues.apache.org/jira/browse/ENTRY-2035
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2035: Metrics should move to destination atomically
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
>  86cae64fd4bbab162a09a1ca27a6d3887bfc2dc1 
> 
> 
> Diff: https://reviews.apache.org/r/63646/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Review Request 63647: SENTRY-2036: sentry_sync_notifications() should set ID when it returns errors

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63647/
---

Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, and 
Sergio Pena.


Bugs: SENTRY-2036
https://issues.apache.org/jira/browse/SENTRY-2036


Repository: sentry


Description
---

SENTRY-2036: sentry_sync_notifications() should set ID when it returns errors


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 71eb9c1b6d02326fc9a7b702652253bd9b57d725 


Diff: https://reviews.apache.org/r/63647/diff/1/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 63250: [SENTRY-1475] SOLR/Sentry authorization plugin (with solr 7)

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63250/#review190372
---


Ship it!




Plesae address the one last comment that I have and we should be good. I have 
updated the jira with the latest patch that is submitted in review borad so 
that test run on the latest patch.

- kalyan kumar kalvagadda


On Nov. 6, 2017, 9:51 p.m., Hrishikesh Gadre wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63250/
> ---
> 
> (Updated Nov. 6, 2017, 9:51 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-1475
> https://issues.apache.org/jira/browse/SENTRY-1475
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> - Upgraded Solr to latest version (v7.1.0)
> - Introduced following new authorizable entity types,
>   - Admin (to authorize various cluster admin operations)
> - Note that privilege configuration for Solr collections admin 
> operations have changed from "collection=admin->action= 
> " to "admin=collections->action=". A migration tool 
> will be provided as part of SENTRY-1480).
>   - Config (to authorize various Solr collection configuration operations)
>   - Schema (to authorize Solr schema changes)
> - Renamed sentry-core-model-search module to sentry-core-model-solr to 
> reflect the fact that it represents logic for Apache SOLR and   is consistent 
> with naming convention used for other components (e.g. kafka)
> - Moved the Solr audit log functionality to sentry-binding-solr module so 
> that it can be integrated with the Solr/Sentryauthorization plugin.
> - Deleted all the custom Solr request handlers in the 
> sentry-solr/solr-sentry-handlers module since the Solr authorization plugin 
> handles the authorization of all solr requests. This module now contains just 
> the functionality required for implementing Solr document level security.
> 
> 
> Diffs
> -
> 
>   pom.xml af54480906489a14788f594da2a923f9e9c0ed29 
>   sentry-binding/sentry-binding-solr/pom.xml 
> ed2624b68322186c10f42c615f635419147ba1f9 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrAuthorizationException.java
>  938dbfdbd9df440b9e6dad574627ebfece6c7f0b 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SentrySolrPluginImpl.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
>  0a818e58c72e52a6d7d60593fd63446402eab2f9 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzUtil.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java
>  37efa5bfda0a2dbc46c38e6cd6c0b8d7b987f865 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/solr/sentry/AuditLogger.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/solr/sentry/RollingFileWithoutDeleteAppender.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/HdfsTestUtil.java
>  859c793c54663af867e710c421929404ef5322e1 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java
>  7a88d905a7dcdf719669c8a14e74b291975606fc 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/AbstractTestSearchPolicyEngine.java
>  3df6ecfd67129af96a1ffe6688738292fc773993 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/AbstractTestSolrPolicyEngine.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/SearchPolicyTestUtil.java
>  e198b5c762ca6eca435d75e07887a10ddd38e361 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/SolrPolicyTestUtil.java
>  PRE-CREATION 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestCollectionRequiredInRole.java
>  76211dd0b30a36a67434f9fef2b4d350a1fab941 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchAuthorizationProviderGeneralCases.java
>  b4aa684358a37530037cd68797a325f1099c3e46 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchAuthorizationProviderSpecialCases.java
>  371f3614a723bd8f515b6d2011e522db5bd7ac51 
>   
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/policy/solr/TestSearchModelAuthorizables.java
>  e7da13ae5cf21a72933f0124d200071ab89379df 
>   
> sentry-binding/sentry-binding-solr/src/tes

Re: Review Request 63646: SENTRY-2035: Metrics should move to destination atomically

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63646/#review190382
---


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Nov. 7, 2017, 9:47 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63646/
> ---
> 
> (Updated Nov. 7, 2017, 9:47 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: ENTRY-2035
> https://issues.apache.org/jira/browse/ENTRY-2035
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2035: Metrics should move to destination atomically
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
>  86cae64fd4bbab162a09a1ca27a6d3887bfc2dc1 
> 
> 
> Diff: https://reviews.apache.org/r/63646/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 63646: SENTRY-2035: Metrics should move to destination atomically

[Vadim Spector via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63646/#review190414
---


Ship it!




Ship It!

- Vadim Spector


On Nov. 7, 2017, 9:47 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63646/
> ---
> 
> (Updated Nov. 7, 2017, 9:47 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Bugs: ENTRY-2035
> https://issues.apache.org/jira/browse/ENTRY-2035
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-2035: Metrics should move to destination atomically
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
>  86cae64fd4bbab162a09a1ca27a6d3887bfc2dc1 
> 
> 
> Diff: https://reviews.apache.org/r/63646/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63645/
---

Review request for sentry, Sergio Pena and Vadim Spector.


Repository: sentry


Description
---

When retrieving full path image update, we split a path by "/" and create HMS 
Path entries. However, the leading "/" presence will cause issues because on 
splitting the value at index 0 will be empty. This will affect the creation of 
HMS path entries.


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
 cef8bd734 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 cf83e7796 


Diff: https://reviews.apache.org/r/63645/diff/1/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore


Thanks,

Arjun Mishra

Re: Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63645/#review190431
---




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 3469 (patched)


While this may be a useful test, it is better to write specific unit test 
for the String manipulation functions directly.


- Alexander Kolbasov


On Nov. 7, 2017, 9:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63645/
> ---
> 
> (Updated Nov. 7, 2017, 9:16 p.m.)
> 
> 
> Review request for sentry, Sergio Pena and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When retrieving full path image update, we split a path by "/" and create HMS 
> Path entries. However, the leading "/" presence will cause issues because on 
> splitting the value at index 0 will be empty. This will affect the creation 
> of HMS path entries.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
>  cef8bd734 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63645/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63645/#review190428
---



Do you also want to update PathUpdate.parsePath() to be consistent?


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 219 (patched)


This comment doesn't belong here - this is utility class which knows 
nothing about HMS - it is about string manipulation.

The relevant omment should say that leading slashes are ignored, so 

/a/b/c beomes {a, b, c}



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Line 219 (original), 224 (patched)


This function now does more then split, so it should be updated and 
documented properly



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Line 220 (original), 226 (patched)


Do we really need to chck for null? Can we specify the non-null value as a 
contract?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 229 (patched)


This function isn't used by anything so why make it public? I suggest to 
just inline it in splitPath()

This whole function can be replaced with
path.replaceAll("^/+", "")



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 230 (patched)


We don't need to collapse slashes any more



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
Lines 232 (patched)


We don't need to replace all since we do path.split("/+"). We only need to 
remove leading slashes, so this check should be changed to startsWith("/")


- Alexander Kolbasov


On Nov. 7, 2017, 9:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63645/
> ---
> 
> (Updated Nov. 7, 2017, 9:16 p.m.)
> 
> 
> Review request for sentry, Sergio Pena and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When retrieving full path image update, we split a path by "/" and create HMS 
> Path entries. However, the leading "/" presence will cause issues because on 
> splitting the value at index 0 will be empty. This will affect the creation 
> of HMS path entries.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
>  cef8bd734 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796 
> 
> 
> Diff: https://reviews.apache.org/r/63645/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>