Re: SQL changes for SENTRY-1726

[Alexander Kolbasov]

The major problem with read-modify-write approach is that it is difficult to 
handle the case with two writers trying to update the value at the same time. 
If you handle this by adding new rows and having the ID as the primary key, one 
writer will succeed and another will fail because the key already exists. How 
would you achieve this in your approach?

- Alex

> On Apr 27, 2017, at 9:14 PM, Kalyan Kumar Kalvagadda  
> wrote:
> 
> Hello all,
> 
> As part of SENTRY-1726, I'm adding new table to store notification-id. HMS
> follower just needs the the latest notification-id that sentry has
> processed.
> All HMS follower needs is the latest notification. As we need not store
> older notification-id's, I defined the table to hold only one record so
> that we can avoid inserting more records in the table and the application
> should just update the existing record.
> 
> 
> CREATE TABLE `SENTRY_LAST_NOTIFICATION_ID`
> (
>`NOTIFICATION_ID` BIGINT NOT NULL,
>`RESTRICTION` VARCHAR(15) NOT NULL DEFAULT 'singleton',
>CONSTRAINT `SENTRY_NOTIFICATION_PK` PRIMARY KEY (`RESTRICTION`)
> )ENGINE=INNODB;
> 
> 
> Application just needs to insert/update the NOTIFICATION_ID. Once we
> insert for the fist time, it is update only.
> 
> On the other hand, If we have NOTIFICATION_ID as primary key and keep
> on inserting NOTIFICATION_ID's into
> 
> the table. we have to use MAX(NOTIFICATION_ID) every time we need the
> notification id. Additionally, we need to build logic to cleanup older
> entries.
> 
> 
> You can refer to review board for complete change set.
> 
> https://reviews.apache.org/r/58808/
> 
> 
> -Kalyan

Re: Review Request 59031: SENTRY-1714 MetastorePlugin.java should quetly return from renameAuthzObject() when both paths are null

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
Lines 416 (patched)
<https://reviews.apache.org/r/59031/#comment247397>

What should happen of one of them is null - either old path or new path?


- Alexander Kolbasov


On May 5, 2017, 9:38 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59031/
> ---
> 
> (Updated May 5, 2017, 9:38 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1714
> https://issues.apache.org/jira/browse/SENTRY-1714
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> MetastorePlugin.java should quietly return from renameAuthzObject() when both 
> paths are null. At least one legitimate case when it happens is with the 
> views, which have no associated oldPath and newPath.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
>  fdb6df4 
> 
> 
> Diff: https://reviews.apache.org/r/59031/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure that sentry metastore plugin is not invoked when view is renamed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59167: SENTRY-1580: Provide pooled client connection model with HA

[Alexander Kolbasov]

> On May 24, 2017, 12:10 p.m., kalyan kumar kalvagadda wrote:
> >

Seems like you have an emty comment.


- Alexander


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


On May 24, 2017, 7:49 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59167/
> ---
> 
> (Updated May 24, 2017, 7:49 a.m.)
> 
> 
> Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na 
> Li, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1580
> https://issues.apache.org/jira/browse/SENTRY-1580
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1580: Provide pooled client connection model with HA
> 
> 
> Diffs
> -
> 
>   pom.xml ad54cfda5e7dabf9ecadc8f8d9f0e3012596fa32 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
>  a3140f2e23188e0bc7b6d5521a2f457d090a937f 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java
>  262db11677c1bb999265a5033971c06def9455ed 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java
>  da587d00aa6e4b2fb6d78e3a720464d25bdcb47c 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
>  fdb6df44953b20dfe655c62d6cbd935b7ba15bf4 
>   
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java
>  48511145098215ce5d077a3d335ac659a2fbbf95 
>   
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
>  240067370b434054addfff00b985fb8c94b2bad9 
>   
> sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java
>  84a61cc0bb5a7d5ba6ca3d74d7bcd3e66a8f351b 
>   sentry-core/sentry-core-common/pom.xml 
> e1be256e9a4b867215afaf94b93cae2cc5e417b1 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  34a594eb24ab9de8ddfa91494e9c2c7d794e1129 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  9ea7185c7a4660a4455a1f6436942043a7f81519 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  651173ebb7df995ff0c432e2ebec08b4d95c3a77 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  2d8082787935bf3111f917761db3bc2d99f59df3 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  c97fe97f67ba6e3ee64785d8baed266db3f7daca 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  9a10ca5aea7fff8a97394bf325fcdfe4418f91a5 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  74aced271dc28aeb294844ec7c0c2b78a0cdb9a3 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/TTransportWrapper.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/TransportFactory.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java
>  d9fab86727919496fd932407b3abf1a9d879f7e0 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryUpdater.java
>  34caa0ea845fa3f57b16212367397b3525e5a785 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java
>  11f6894820f1df3102114787fa5fe2c060619541 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  798bbef98b29af7c885c5ea747d77d2dad6c1693 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
>  e350103cfc31

Re: Review Request 59510: SENTRY-1774 HMSFollower should always depend on persisted information to decide is full snapshot is needed

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2814 (patched)
<https://reviews.apache.org/r/59510/#comment249331>

According to the spec, getFirstMAuthzPathsMappingCore never returns null, 
so the first check is not needed and the second check is cleaner with using 
isEmpty().



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2815 (patched)
<https://reviews.apache.org/r/59510/#comment249332>

Please do not use if statements to return true/false values, return 
conditions directly.

return some_boolean_condition



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 97 (original), 96 (patched)
<https://reviews.apache.org/r/59510/#comment249329>

With your change, why do we still need this?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 103 (original), 101 (patched)
<https://reviews.apache.org/r/59510/#comment249330>

This return is redundand now


- Alexander Kolbasov


On May 24, 2017, 1:36 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59510/
> ---
> 
> (Updated May 24, 2017, 1:36 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, 
> Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1774
> https://issues.apache.org/jira/browse/SENTRY-1774
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> HMSFollower should always depend on persisted information to decide is full 
> snapshot is needed instead of depending on in-memory information.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  8450402 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  58e8881 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  440b0e9 
> 
> 
> Diff: https://reviews.apache.org/r/59510/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test to check new functionality added and also ran HMSFollower tests
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59424: SENTRY-1736 Generic service client should support Kerberos(Continuation Fix)

[Alexander Kolbasov]

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



The SENTRY-1736 is already comitted and resolved. Please file another JIRA for 
subsequent changes.

- Alexander Kolbasov


On May 19, 2017, 11:27 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59424/
> ---
> 
> (Updated May 19, 2017, 11:27 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1736
> https://issues.apache.org/jira/browse/SENTRY-1736
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Older patch had issues as the UserGroupInformation was initialized when the 
> first connection was created. This will not solve the issue as the connection 
> is not created when the sentry client is created. New logic which initialize 
> UserGroupInformation when the first client is created.
> 
> This patch is continuation of older patch as the older patch is already 
> committed. This change set also contains tests to make sure things are as 
> expected.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  f609d33 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceClientForUgi.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClientForUgi.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClientForUgi.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59424/diff/1/
> 
> 
> Testing
> ---
> 
> Added new tests for this. Test Solr command line till in real cluster.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59793: SENTRY-1780: FullUpdateInitializer does not kill the threads whenever getFullHMSSnapshot throws an...

[Alexander Kolbasov]

> On June 5, 2017, 3:11 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 390 (patched)
> > <https://reviews.apache.org/r/59793/diff/2/?file=1741031#file1741031line395>
> >
> > what is the benefit of catching exception here?
> > 
> > Its caller catches the exception and logs it. Providing more info on 
> > the failure.

The exception was already logged. The only possible reaction is to retry the 
same thing again - the caller can't do anything useful with it.


- Alexander


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


On June 4, 2017, 6:48 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59793/
> ---
> 
> (Updated June 4, 2017, 6:48 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1780
> https://issues.apache.org/jira/browse/SENTRY-1780
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1780: FullUpdateInitializer does not kill the threads whenever 
> getFullHMSSnapshot throws an exception
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  efd3fa3d24b4160812f02ccc89c39ec17f7b367e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  d410a6ce3bd535823ca656ab1fc879fd38336922 
> 
> 
> Diff: https://reviews.apache.org/r/59793/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 59167: SENTRY-1580: Provide pooled client connection model with HA

[Alexander Kolbasov]

 75d2993a4a7a86b99f206a82b9fe751e79a04adb 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
 134012d18d75d12e3ccce82dac8145bfc41609f5 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java
 41708c371b86e000e94ac78247bfb3bfc6c4a252 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java
 11cdee7f1be13fae10028e7ab20e0a3b06e75237 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
 e23d13ba21b1042a7e73dd8807018fa9899be609 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java
 46ac4a3348f50ab1c02e09f3e1eea59690392f4e 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java
 404adb8fa612d5e1026a0cf96a65a98db4ca10c3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java
 d6d9014a90b9171d02363d0f9e651891ba8c531d 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java
 695c008c03c927b1126423bb61c52f5e3c5b18b9 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
 c2b03e5d62b88bd5c2f696af8f8872c42c02de9c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 d1a4d99f8080fc6178770986acdd101d6fe997f7 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java
 1d0984671ab5014b0a0ace263b7f9967e013b7ee 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
 acf9b05281d8456ebdf77414dc7886f9aa4ef28f 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 9beb07b43b405725f6f5e9b5d0d5c36f22f4cad8 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java
 7db93101c7f7534ea2c6e67ae5d661cbca8411b1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java
 0164fa6197822bf450754dc1fd116581c2321d0f 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClientForUgi.java
 3f84ae4e92910142c683ebfd46351cb80578a58a 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClientForUgi.java
 ef9459857eb4504b50dd838fc79e2e807b017467 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java
 a4dd8a65634ce3fb07952782fb8fdc4d99e7f3f9 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java
 a2027750add889e31ec1bb9b2a287d046e6c6343 
  
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java
 bead00309f392b916c1ef5983248dedce3b99521 
  
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java
 0b1ef68a55a1d40637f771759735fc0838525a40 
  
sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java
 8a01e1c5c4ec58eded5621ad94ee28bc68d66c0b 


Diff: https://reviews.apache.org/r/59167/diff/9/

Changes: https://reviews.apache.org/r/59167/diff/8-9/


Testing
---

I used the interactive shell to test failover across two different servers.


Thanks,

Alexander Kolbasov

Re: Optimizing Sentry to HDFS protocol

[Alexander Kolbasov]

Lina, thanks for your comments!

> On Jun 27, 2017, at 9:33 PM, Na Li <lina...@cloudera.com> wrote:
> 
> Sasha,
> 
> 1) "- Rather then streaming huge snapshots in a single message we should
>>  provide streaming protocol with smaller messages and later reassembly on
>>  the HDFS side."
> Based on https://thrift.apache.org/docs/concepts, Thrift transport can be raw 
> TCP or HTTP. HTTP is above TCP. TCP will cut application stream into blocks 
> that can fit into IP packets, which can fit into link layer frames. So 
> application (such as Sentry) does not need to handle such low level 
> processing, such as cutting stream into small messages and then reassemble 
> into stream again. What is the reason you want to do that? Did you see 
> performance issue? We can capture packets on the wire and see the exact 
> protocol stack of Thrift, and decide if we want to change configuration to 
> improve performance.

If the source data lives as a set of record in the DB, how can you send send it 
with Thrift without first creating an in-memory image of the whole dataset? The 
idea is to be able to stream almost directly from DB to the other side with 
very little memory consumption.

The current implementation builds the whole representation in memory as a set 
of not very efficient data structures, then serializes it into an in-memory 
buffer, so you need twice as much memory and then sends it. The receiver needs 
to do the opposite.

> 
> 2) "  - Most of the information passed are long strings with common prefixes.
>> 
>>  We should be able to apply simple compression techniques (e.g. prefix
>>  compression) or even run a full compression on the data before sending."
> Bas d on http://thrift-tutorial.readthedocs.io/en/latest/thrift-stack.html, 
> Thrift supports compression. We can configure its protocol as TDenseProtocol 
> or TCompactProtocol.

Looking at the Compact protocol it doesn’t provide real compression. And since 
we know what kind of data we are dealing with (sets of HDFS paths) we can 
provide very compact representations.


> 
> 3) "  - We should consider using non-thrift data structures for passing the
>> 
>>  info and just use Thrift as a transport mechanism."
> What is the reason you want to make this change? 
> Based on 
> https://stackoverflow.com/questions/9732381/why-thrift-why-not-http-rpcjsongzip,
>  Thrift has several benefits.

Because these structures are not memory-efficient.In your stack overflow 
article it talks about benefits of Thrift over HTTP. Here we are talking about 
internal representation, while keeping Thrift transport.

> 
> Thanks,
> 
> Lina
> 
> Sent from my iPhone
> 
>> On Jun 27, 2017, at 5:44 PM, Alexander Kolbasov <ak...@cloudera.com> wrote:
>> 
>> Some food for thought.
>> 
>> Currently Sentry uses serialized Thrift structures to send a lot of
>> information from the Sentry Server to the HDFS namenode plugin for the HDFS
>> sync.
>> 
>> We should think of ways to optimize this protocol in several ways:
>> 
>> 
>>  - Rather then streaming huge snapshots in a single message we should
>>  provide streaming protocol with smaller messages and later reassembly on
>>  the HDFS side.
>>  - Most of the information passed are long strings with common prefixes.
>>  We should be able to apply simple compression techniques (e.g. prefix
>>  compression) or even run a full compression on the data before sending.
>>  - We should consider using non-thrift data structures for passing the
>>  info and just use Thrift as a transport mechanism.
>> 
>> - Sasha

Review Request 60490: SENTRY-1820: Add JSON file reporter for Sentry metrics

[Alexander Kolbasov]

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

Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
Vamsee Yarlagadda.


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


Repository: sentry


Description
---

SENTRY-1820: Add JSON file reporter for Sentry metrics


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 e3691a96ef1142fdae74ba873683a025265cc006 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 d3c96fafc262ca7cfc3ba97ea24c715ab0b129ac 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62231: SENTRY-1946: getPathsUpdatesFrom() got its boolean logic inversed which results in sending two snapshots at the same time

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 12, 2017, 1 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62231/
> ---
> 
> (Updated Sept. 12, 2017, 1 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1946: Conditional error in SENTRY-1931 results in sending two 
> snapshots at the same time
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  2866ab348390e092d0c1732c6ab8a8bea0529a4b 
> 
> 
> Diff: https://reviews.apache.org/r/62231/diff/1/
> 
> 
> Testing
> ---
> 
> In progress
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Re: Review Request 62221: SENTRY-1938: Sentry logs to provide more relevant information

[Alexander Kolbasov]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 53 (patched)
<https://reviews.apache.org/r/62221/#comment261611>

Hmm, isn't this what methods are for? Why not have 

public String retrieverType()

method?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 74 (original), 79 (patched)
<https://reviews.apache.org/r/62221/#comment261613>

WHy do you want to combine String format with Logger formatting? You may 
just use String format to get all three. 

And the log can be simply

`Get all %s updates for image %d from sequence %d`



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 79 (original), 84 (patched)
<https://reviews.apache.org/r/62221/#comment261615>

Suggested format here and other places is 

`(PATH) something something`

Why not

`Current PATH image ...`



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 95 (patched)
<https://reviews.apache.org/r/62221/#comment261614>

can it be null?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 143 (patched)
<https://reviews.apache.org/r/62221/#comment261620>

If you are going to the trouble of logging it, please also log pathSeqNum 
and pathImgNum



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 201 (original), 203 (patched)
<https://reviews.apache.org/r/62221/#comment261619>

We go to so much trouble to print this message and I don't see why it is 
useful. The important part is the advance of notification IDs that can be 
tracked through metrics. What if it successfully processed one message but not 
the rest? How would this message help someone?


- Alexander Kolbasov


On Sept. 13, 2017, 2:26 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62221/
> ---
> 
> (Updated Sept. 13, 2017, 2:26 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Na Li, Sergio 
> Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry Supportability improvement.
> 
> 1. HMSFollower 
> * Print confirmation message (at INFO level) once full snapshot is persisted 
> in the DB.
> * Print the message that HMSFollower is completely ready (after the initial 
> pass of HMSFollower is done)
> 
> 2. DBUpdateForwarder 
> * Every log message should explicitly mention which type of events is it 
> referring to (PERM or PATH) otherwise there is no way for us to differentiate 
> between calls.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  8a34d5623 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  1318082d3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  31fd4597d 
> 
> 
> Diff: https://reviews.apache.org/r/62221/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: [DISCUSS] Allow JDK8 specific features on Sentry 2.0

[Alexander Kolbasov]

Bump - does anyone have any opinions on this based on the information
provided by Sergio?

On Fri, Sep 1, 2017 at 9:07 AM, Sergio Pena <sergio.p...@cloudera.com>
wrote:

> The Hive community started the discussion to support JDK8 in apr/2016 for
> Hive 2.x versions. There was a mix of decisions to whether use JDK8 100% or
> just keep compatibility for JDK8. The final decision was to build and test
> with JDK8 on Hive 2.0 but keep JDK7 compatibility for one release. The Hive
> community had some active maintenance releases as well, so I think one of
> the decisions was to keep backports to maintenance releases easy.
> https://lists.apache.org/thread.html/83d8235bc9547cc94a0d689580f20d
> b4b946876b6d0369e31ea12b51@1460158490@%3Cdev.hive.apache.org%3E
>
> Later on feb/2017, the community started a vote to start using JDK8 full
> features and drop support for JDK7. The vote passed, and we started dropped
> JDK7 support on Hive 2.1. However, the community didn't introduce any JDK8
> features yet even they were allowed.
> https://lists.apache.org/thread.html/dcd57844ceac7faf8975a00d5b8b18
> 25ab5544d94734734aedc3840e@%3Cdev.hive.apache.org%3E
>
> Recently in newer Hive 2.x releases and master, the community has started
> to use these features. My experience hasn't been bad on backports as not
> all developers use these new features (including me). There are other
> backports issues regarding new code base for certain features that makes us
> hard to backport, but we haven't had any problem with JDK8 at all.
>
> On Thu, Aug 31, 2017 at 6:19 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
> > Sergio, can you share Apache Hive experience with this issue?
> >
> > Thanks,
> >
> > - Alex
> >
> > On Thu, Aug 31, 2017 at 1:10 PM, Sergio Pena <sergio.p...@cloudera.com>
> > wrote:
> >
> > > Hi All,
> > >
> > > This thread discussion is a follow-up to the old thread related to
> > > supporting JDK8 and Datanucleus 4 as the minimum version for Sentry
> 2.0.
> > > This is dedicated to discuss whether we should allow using JDK8
> specific
> > > features (such as lambda functions and other useful API) in Sentry.
> > >
> > > Advantages are that we could start using these cool features that come
> in
> > > JDK8 and forget about JDK7 for all.
> > >
> > > Disadvantages are that doing backports on older releases and/or
> allowing
> > > other companies backporting fixes from Sentry 2.x will make these
> > backports
> > > harder because JDK7 is still in use.
> > >
> > > Maintenance releases are not active but companies are still pretty
> active
> > > on Sentry.
> > >
> > > Questions to answer:
> > > - What should we do?
> > > - If we decide to keep JDK7 compatibility, how long should we keep this
> > > until we move completely to JDK8?
> > >
> > > In my opinion, companies will always be outdated with what we do as an
> > > Apache community. Taking a look at what we are doing with SentryHA
> > > redesign, this is a breaking change for companies too because fixes on
> > this
> > > new design may not work for other companies and/or the backports could
> be
> > > harder.
> > >
> > > However, the current Sentry 2.0 has been active with JDK7 as the
> support,
> > > and users active on 2.0 may be using JDK7 environments only.
> > >
> > > So,
> > > Should we wait until Sentry 2.1 or newer releases to allow JDK8
> features?
> > > Should we start in Sentry 2.0?
> > >
> > > Any thoughts?
> > >
> > > - Sergio
> > >
> >
>

Re: Review Request 62409: SENTRY-1949: Old full snapshots are never cleaned up

[Alexander Kolbasov]

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



How much time does it take to clean up for a huge snapshot? We already taking a 
big hit for taking it, so I wondering if we should consider doing this as part 
of our existing cleanup thread.

- Alexander Kolbasov


On Sept. 19, 2017, 2:51 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62409/
> ---
> 
> (Updated Sept. 19, 2017, 2:51 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: sentry-1949
> https://issues.apache.org/jira/browse/sentry-1949
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added a new method on SentryStore to delete HMS snapshots since a specific 
> ID. This new method is called by the HMSFollower when a new snapshot is 
> created.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  9a37ac8fe572a98b3fb4cba127bcb1716bd8f017 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  31fd4597dad6b02bb5d8b9f944a488f6f78d7d79 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796fd5bc22c76cba6096b0a821e82b87a7 
> 
> 
> Diff: https://reviews.apache.org/r/62409/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62409: SENTRY-1949: Old full snapshots are never cleaned up

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2844 (patched)
<https://reviews.apache.org/r/62409/#comment262034>

The comment (and the name) are a bit confusing. I would think that we are 
deleting all snapshot starting from the one provided and going forward, but you 
are deleting them going backwards.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2867 (patched)
<https://reviews.apache.org/r/62409/#comment262035>

Hmm, this comment is very criptic. What are you trying to say?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 335 (patched)
<https://reviews.apache.org/r/62409/#comment262050>

I think it makes sense to ignore exception during cleanup.


- Alexander Kolbasov


On Sept. 19, 2017, 2:51 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62409/
> ---
> 
> (Updated Sept. 19, 2017, 2:51 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: sentry-1949
> https://issues.apache.org/jira/browse/sentry-1949
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added a new method on SentryStore to delete HMS snapshots since a specific 
> ID. This new method is called by the HMSFollower when a new snapshot is 
> created.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  9a37ac8fe572a98b3fb4cba127bcb1716bd8f017 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  31fd4597dad6b02bb5d8b9f944a488f6f78d7d79 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  cf83e7796fd5bc22c76cba6096b0a821e82b87a7 
> 
> 
> Diff: https://reviews.apache.org/r/62409/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 62496: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1963: Sentry JSON reporter should use regular implementation for local 
file system


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 4063a663c86ea5ed88c40013d5b3550ac0a6272f 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryMetrics.java
 PRE-CREATION 


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


Testing
---

New unit test written


Thanks,

Alexander Kolbasov

Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system

[Alexander Kolbasov]

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

(Updated Sept. 22, 2017, 8:19 a.m.)


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


Changes
---

Removed unused imports


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


Repository: sentry


Description
---

SENTRY-1963: Sentry JSON reporter should use regular implementation for local 
file system


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 4063a663c86ea5ed88c40013d5b3550ac0a6272f 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryMetrics.java
 PRE-CREATION 


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

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


Testing
---

New unit test written


Thanks,

Alexander Kolbasov

Re: Review Request 62411: SENTRY-1958: Bump to Hive version 2.0

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 21, 2017, 12:59 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62411/
> ---
> 
> (Updated Sept. 21, 2017, 12:59 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colm O hEigeartaigh, and 
> kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1958
> https://issues.apache.org/jira/browse/sentry-1958
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch bumps the Hive version of hive-authz1 to Hive 2.0. Moving to 
> authz2 has been a little complicated, so doing an incremental patch was 
> preffered. 
> Also, we're taking advantage of the unit tests nad e2e tests that already 
> exist on Sentry. There are tests that are not on the authz2, so this is why I 
> preffered to look into bumping the Hive 2.0 version first.
> 
> The following issues with Hive 2.0 were found and addressed on Sentry:
> - Hive 2.0.1 has a bug that Sentry cannot workaround.
> - Hive 2.1.1 and higher has a different bug that Sentry cannot workaround.
> - Hive CBO has a bug where ReadIdentity partitions do not have parents 
> causing Sentry to request extra privileges that the user might not have  
>   CBO is disabled on the Sentry tests and it must be disabled on production 
> as well.
> - HIVE-11145: Remove OFFLINE and NO_DROP from tables and partitions
>   Removed tests that use the protection mode operations as Hive do not 
> support them any more.
> - HIVE-10453: HS2 leaking open file descriptors when using UDFs
>   Hive 2.0 clears all functions after a session is closed causing other users 
> who want to execute such
>   function to fail because they lost the function scope and they do not have 
> permissions to create functions
> - HIVE-12320: hive.metastore.disallow.incompatible.col.type.changes should be 
> true by default
>   Sentry had some issues on the tests due to this Hive change.
> - HIVE-10307: Support to use number literals in partition column
>   Hive 2.0 added an extra validation when using ALTER TABLE ... PARTITION 
> (spec) that throws an error if
>   spec is not a partition column.
> - The HS2 webui fails to start when concurrency mode is enabled. The Sentry 
> tests are now configured to put
>   Hive in test mode so that the webui is not initialized.  
> - There are some column names that cannot be used as they are reservered by 
> Hive, i.e 'date' column name fails
>   in some Sentry tests. 
> - Hive 2.0 switched to log4j2 causing some Sentry tests to fail.
> 
> Important changes on Sentry:
> - Hive 2.0 has an authz1 bug with the use of SentryMetastoreFilterHook class. 
> This class is replaced automatically by Hive with a default one that uses 
> authz2. To make minimal changes on Sentry, a new class is created 
> (SentryHiveAuthorizerImpl) that only deals with metastore filtering. Also, 
> the class SentryHiveAuthorizerFactory is set by the 
> HiveAuthzBindingSessionHook automatically when a HS2 session is opened. 
> However, this new authorizer must be set manually on the hive-site.xml so 
> that other clients who use the HMS Client can use this new filter class (info 
> is mentioned on the SentryHiveAuthorizerFactory class.
> 
> There will be follow-up patches to fix the following:
> - Remove SentryMetastoreFilterHook and improve the SentryHiveAuthorizerImpl 
> to avoid the overhead of converting a list of hive privileges to a list of 
> strings and viceversa.
> - Merge authz1 and authz2 dependencies.
> - Review current authz2 and merge code into the new changes.
> 
> 
> Diffs
> -
> 
>   dev-support/test-patch.py ac91b590c93884911d27928ac60108dcaa5d81ed 
>   pom.xml 53679f90b96bbd4db5f0ac004d0de542e439c7f7 
>   sentry-binding/sentry-binding-hive-follower-v2/pom.xml 
> fa7e9282592bf5fe15a35921b7173c81b45a1f1f 
>   sentry-binding/sentry-binding-hive-follower/pom.xml 
> e69519cc63d5c89db6098fa169089b34dbaadae8 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
>  1e636c94afc99678658bdafe74fdd7aff7e12d92 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONDropPartitionMessage.java
>  d3ebf603f2d81b3fed7ec0d33031446185b80cb5 
>   
> sentry-binding/s

Re: Review Request 62411: SENTRY-1958: Bump to Hive version 2.0

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java
Lines 22 (patched)
<https://reviews.apache.org/r/62411/#comment262127>

This class isn't used in 2.0 so there is no need to do anything special 
with it.


- Alexander Kolbasov


On Sept. 19, 2017, 3:10 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62411/
> ---
> 
> (Updated Sept. 19, 2017, 3:10 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colm O hEigeartaigh, and 
> kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1958
> https://issues.apache.org/jira/browse/sentry-1958
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch bumps the Hive version of hive-authz1 to Hive 2.0. Moving to 
> authz2 has been a little complicated, so doing an incremental patch was 
> preffered. 
> Also, we're taking advantage of the unit tests nad e2e tests that already 
> exist on Sentry. There are tests that are not on the authz2, so this is why I 
> preffered to look into bumping the Hive 2.0 version first.
> 
> The following issues with Hive 2.0 were found and addressed on Sentry:
> - Hive 2.0.1 has a bug that Sentry cannot workaround.
> - Hive 2.1.1 and higher has a different bug that Sentry cannot workaround.
> - Hive CBO has a bug where ReadIdentity partitions do not have parents 
> causing Sentry to request extra privileges that the user might not have  
>   CBO is disabled on the Sentry tests and it must be disabled on production 
> as well.
> - HIVE-11145: Remove OFFLINE and NO_DROP from tables and partitions
>   Removed tests that use the protection mode operations as Hive do not 
> support them any more.
> - HIVE-10453: HS2 leaking open file descriptors when using UDFs
>   Hive 2.0 clears all functions after a session is closed causing other users 
> who want to execute such
>   function to fail because they lost the function scope and they do not have 
> permissions to create functions
> - HIVE-12320: hive.metastore.disallow.incompatible.col.type.changes should be 
> true by default
>   Sentry had some issues on the tests due to this Hive change.
> - HIVE-10307: Support to use number literals in partition column
>   Hive 2.0 added an extra validation when using ALTER TABLE ... PARTITION 
> (spec) that throws an error if
>   spec is not a partition column.
> - The HS2 webui fails to start when concurrency mode is enabled. The Sentry 
> tests are now configured to put
>   Hive in test mode so that the webui is not initialized.  
> - There are some column names that cannot be used as they are reservered by 
> Hive, i.e 'date' column name fails
>   in some Sentry tests. 
> - Hive 2.0 switched to log4j2 causing some Sentry tests to fail.
> 
> Important changes on Sentry:
> - Hive 2.0 has an authz1 bug with the use of SentryMetastoreFilterHook class. 
> This class is replaced automatically by Hive with a default one that uses 
> authz2. To make minimal changes on Sentry, a new class is created 
> (SentryHiveAuthorizerImpl) that only deals with metastore filtering. Also, 
> the class SentryHiveAuthorizerFactory is set by the 
> HiveAuthzBindingSessionHook automatically when a HS2 session is opened. 
> However, this new authorizer must be set manually on the hive-site.xml so 
> that other clients who use the HMS Client can use this new filter class (info 
> is mentioned on the SentryHiveAuthorizerFactory class.
> 
> There will be follow-up patches to fix the following:
> - Remove SentryMetastoreFilterHook and improve the SentryHiveAuthorizerImpl 
> to avoid the overhead of converting a list of hive privileges to a list of 
> strings and viceversa.
> - Merge authz1 and authz2 dependencies.
> - Review current authz2 and merge code into the new changes.
> 
> 
> Diffs
> -
> 
>   dev-support/test-patch.py ac91b590c93884911d27928ac60108dcaa5d81ed 
>   pom.xml 1479f5fa3895833a1efa63d0f4a7c3db72c7cde9 
>   sentry-binding/sentry-binding-hive-follower-v2/pom.xml 
> fa7e9282592bf5fe15a35921b7173c81b45a1f1f 
>   sentry-binding/sentry-binding-hive-follower/pom.xml 
> e69519cc63d5c89db6098fa169089b34dbaadae8 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
>  1e636c94afc99678658

Re: Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

> On Sept. 13, 2017, 8:48 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 1798-1801 (original), 1816-1819 (patched)
> > <https://reviews.apache.org/r/62261/diff/3/?file=1820454#file1820454line1831>
> >
> > Asking for all groups at once could save multiple hops to the database.
> > 
> > ```bash
> > private Set getRoleNamesForGroupsCore(PersistenceManager pm, 
> > Set groups) {
> > Query query = pm.newQuery(MSentryGroup.class);
> > query.setFilter(":p1.contains(this.groupName)");
> > List sentryGroups = (List) 
> > query.execute(groups.toArray());
> > if (sentryGroups.isEmpty()) {
> >   return Collections.emptySet();
> > }
> > Set result = new HashSet<>();
> > for (MSentryGroup sentryGroup : sentryGroups) {
> >   for (MSentryRole role : sentryGroup.getRoles()) {
> > result.add(role.getRoleName());
> >   }
> > }
> > return result;
> >   }
> > ```

I tried this approach and don't see any benefit - in fact it is slightly worse. 
More over it breaks the semantics where exception is thrown where group isn't 
found.


- Alexander


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


On Sept. 13, 2017, 3:49 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62261/
> ---
> 
> (Updated Sept. 13, 2017, 3:49 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1937
> https://issues.apache.org/jira/browse/SENTRY-1937
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1937 Optimize performance for listing sentry roles by group name
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  01a7c830f548d41062fb3bbd0a81f71514585aa5 
> 
> 
> Diff: https://reviews.apache.org/r/62261/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system

[Alexander Kolbasov]

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

(Updated Sept. 22, 2017, 6:28 p.m.)


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


Changes
---

Addressed code review comment from Lina plus more comments


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


Repository: sentry


Description
---

SENTRY-1963: Sentry JSON reporter should use regular implementation for local 
file system


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 4063a663c86ea5ed88c40013d5b3550ac0a6272f 


Diff: https://reviews.apache.org/r/62496/diff/3/

Changes: https://reviews.apache.org/r/62496/diff/2-3/


Testing
---

New unit test written


Thanks,

Alexander Kolbasov

Re: Review Request 62393: SENTRY-1943: Update Guava to 14.0

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 18, 2017, 10:34 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62393/
> ---
> 
> (Updated Sept. 18, 2017, 10:34 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Bugs: sentry-1943
> https://issues.apache.org/jira/browse/sentry-1943
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Update Guava to a version that works with JDK7.
> 
> 
> Diffs
> -
> 
>   pom.xml 1479f5fa3895833a1efa63d0f4a7c3db72c7cde9 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java
>  2396c972b77fce97513155acd161fd5109faf620 
> 
> 
> Diff: https://reviews.apache.org/r/62393/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Proposal to have sentry 1.9 release.

[Alexander Kolbasov]

I think the problem is that sentry 1 must work with hive 1 as well.

On Fri, Oct 6, 2017 at 07:32 Kalyan Kumar Kalvagadda <kkal...@cloudera.com>
wrote:

> Sasha,
>
> You got me wrong. I'm proposed to go with Hive 2.0 using authv1 for sentry
> 1.9.0 release.
>
> -Kalyan
>
> -Kalyan
>
> On Thu, Oct 5, 2017 at 5:52 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
> > Sergio - what is the state of HMS notifications in Hive1? Do they even
> > exist there?
> >
> > Kalyan - I am not sure that the new Sentry architecture which is based on
> > HMS notifications can work with Hive 1 at all.
> >
> > On Thu, Oct 5, 2017 at 2:52 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > Sasha,
> > >
> > > Proposal was to release sentry 1.9.0 which will be branched out of
> > current
> > > master which works with Hive 2.0 using authv1.
> > > Hive 2.0 has a lot fixes for HMS notification as well.
> > >
> > > I agree, i think it would miss one commit in Hive which is needed for
> > > sentryHa which is support for "SentryHMSSyncPostEventListener". This
> fix
> > > is
> > > used to synchronize the requests when the object creation and privilege
> > > addition are automated and are executed back to back. Can't we can have
> > it
> > > documented as a known issue.
> > >
> > >
> > > -Kalyan
> > >
> > >
> > >
> > >
> > > -Kalyan
> > >
> > > On Thu, Oct 5, 2017 at 3:02 PM, Alexander Kolbasov <ak...@cloudera.com
> >
> > > wrote:
> > >
> > > > The major reason why we wanted to move to Sentry 2.0 was the
> dependency
> > > on
> > > > Hive version. Sentry HA completely relies on HMS notifications which
> > are
> > > > not available in Hive 1, more over it relies on fixes that are only
> > > > available in Hive 2.4. Sentry 1 should work with Hive 1, so we can't
> > make
> > > > Sentry HA work with Hive 1.
> > > >
> > > > - Sasha
> > > >
> > > > On Thu, Oct 5, 2017 at 7:15 AM, Kalyan Kumar Kalvagadda <
> > > > kkal...@cloudera.com> wrote:
> > > >
> > > > > Colm,
> > > > >
> > > > > If we have consensus on releasing sentry 1.9.0, I can work on
> release
> > > > > process which takes couple of weeks from now.
> > > > >
> > > > > -Kalyan
> > > > >
> > > > > On Thu, Oct 5, 2017 at 3:50 AM, Colm O hEigeartaigh <
> > > cohei...@apache.org
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Fine with me, it makes sense to get the HA work out there if it
> is
> > > > ready
> > > > > to
> > > > > > be used. What kind of time-line are you thinking of for 1.9.0?
> > > > > >
> > > > > > Colm.
> > > > > >
> > > > > > On Thu, Oct 5, 2017 at 4:37 AM, Sergio Pena <
> > > sergio.p...@cloudera.com>
> > > > > > wrote:
> > > > > >
> > > > > > > It sounds good to do. One of the reasons to do sentry 2.0 was
> the
> > > > > > > integration with hive-authz2, but this is taking time for
> quality
> > > > > > concerns.
> > > > > > > Perhaps is good to cut the branch for 1.9 now before starting
> to
> > > add
> > > > > > other
> > > > > > > incompatible changes to the master branch.
> > > > > > >
> > > > > > > +1 to release 1.9 + ha work.
> > > > > > >
> > > > > > > On Wed, Oct 4, 2017 at 5:19 PM, Kalyan Kumar Kalvagadda <
> > > > > > > kkal...@cloudera.com> wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I understand that the major areas that still needs
> significant
> > > > amount
> > > > > > > work
> > > > > > > > on sentry 2.0 are
> > > > > > > >
> > > > > > > >1. Integrating with Hive 2.0
> > > > > > > >2. Updating the java version
> > > > > > > >3. Integrating with Solr-7
> > > > > > > >
> > > >

Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system

[Alexander Kolbasov]

> On Oct. 2, 2017, 3:11 p.m., kalyan kumar kalvagadda wrote:
> > Sasha, Could you please explain "what does regular implementation for local 
> > file system" mean? That will help get the conetxt what you are trying to do.

The original implementation uses Hadoop local filesystem interface which isn't 
needed here at all - we are much better off using java.nio file system 
operations instead.


- Alexander


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


On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> ---
> 
> (Updated Sept. 22, 2017, 6:28 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar 
> kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local 
> file system
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
>  4063a663c86ea5ed88c40013d5b3550ac0a6272f 
> 
> 
> Diff: https://reviews.apache.org/r/62496/diff/3/
> 
> 
> Testing
> ---
> 
> New unit test written
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system

[Alexander Kolbasov]

> On Sept. 22, 2017, 3:55 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
> > Line 319 (original), 337 (patched)
> > <https://reviews.apache.org/r/62496/diff/2/?file=1832780#file1832780line346>
> >
> > This is similar to "tmpFile = Files.createTempFile(tmpDir, "hmetrics", 
> > "json", FILE_ATTRS);" in https://reviews.apache.org/r/62487/diff/2#0
> > 
> > This means both temporary file has the same name. Is the temDir of the 
> > same value?
> > 
> > SENTRY_JSON_REPORTER_FILE_DEFAULT = "/tmp/sentry-metrics.json"; 
> > 
> > This means the temDir is /tmp, so sentry temp metric file 
> > will be of the same name with hive temp metric file. Is this correct?
> > 
> > Can we use the temFile name defined in 
> > ServerConfig.SENTRY_JSON_REPORTER_FILE and   
> > ServerConfig.SENTRY_JSON_REPORTER_FILE_DEFAULT?

createTempFile creates a file with unique name every time it is called. but we 
can change prefix as well to be clear what this file is.


- Alexander


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


On Sept. 22, 2017, 6:28 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62496/
> ---
> 
> (Updated Sept. 22, 2017, 6:28 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar 
> kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1963
> https://issues.apache.org/jira/browse/SENTRY-1963
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1963: Sentry JSON reporter should use regular implementation for local 
> file system
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
>  4063a663c86ea5ed88c40013d5b3550ac0a6272f 
> 
> 
> Diff: https://reviews.apache.org/r/62496/diff/3/
> 
> 
> Testing
> ---
> 
> New unit test written
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Proposal to have sentry 1.9 release.

[Alexander Kolbasov]

Sergio - what is the state of HMS notifications in Hive1? Do they even
exist there?

Kalyan - I am not sure that the new Sentry architecture which is based on
HMS notifications can work with Hive 1 at all.

On Thu, Oct 5, 2017 at 2:52 PM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> Sasha,
>
> Proposal was to release sentry 1.9.0 which will be branched out of current
> master which works with Hive 2.0 using authv1.
> Hive 2.0 has a lot fixes for HMS notification as well.
>
> I agree, i think it would miss one commit in Hive which is needed for
> sentryHa which is support for "SentryHMSSyncPostEventListener". This fix
> is
> used to synchronize the requests when the object creation and privilege
> addition are automated and are executed back to back. Can't we can have it
> documented as a known issue.
>
>
> -Kalyan
>
>
>
>
> -Kalyan
>
> On Thu, Oct 5, 2017 at 3:02 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
>
> > The major reason why we wanted to move to Sentry 2.0 was the dependency
> on
> > Hive version. Sentry HA completely relies on HMS notifications which are
> > not available in Hive 1, more over it relies on fixes that are only
> > available in Hive 2.4. Sentry 1 should work with Hive 1, so we can't make
> > Sentry HA work with Hive 1.
> >
> > - Sasha
> >
> > On Thu, Oct 5, 2017 at 7:15 AM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > Colm,
> > >
> > > If we have consensus on releasing sentry 1.9.0, I can work on release
> > > process which takes couple of weeks from now.
> > >
> > > -Kalyan
> > >
> > > On Thu, Oct 5, 2017 at 3:50 AM, Colm O hEigeartaigh <
> cohei...@apache.org
> > >
> > > wrote:
> > >
> > > > Fine with me, it makes sense to get the HA work out there if it is
> > ready
> > > to
> > > > be used. What kind of time-line are you thinking of for 1.9.0?
> > > >
> > > > Colm.
> > > >
> > > > On Thu, Oct 5, 2017 at 4:37 AM, Sergio Pena <
> sergio.p...@cloudera.com>
> > > > wrote:
> > > >
> > > > > It sounds good to do. One of the reasons to do sentry 2.0 was the
> > > > > integration with hive-authz2, but this is taking time for quality
> > > > concerns.
> > > > > Perhaps is good to cut the branch for 1.9 now before starting to
> add
> > > > other
> > > > > incompatible changes to the master branch.
> > > > >
> > > > > +1 to release 1.9 + ha work.
> > > > >
> > > > > On Wed, Oct 4, 2017 at 5:19 PM, Kalyan Kumar Kalvagadda <
> > > > > kkal...@cloudera.com> wrote:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > I understand that the major areas that still needs significant
> > amount
> > > > > work
> > > > > > on sentry 2.0 are
> > > > > >
> > > > > >1. Integrating with Hive 2.0
> > > > > >2. Updating the java version
> > > > > >3. Integrating with Solr-7
> > > > > >
> > > > > >
> > > > > > *Integrating with Hive 2.0: *While testing stuff with Hive
> locally,
> > > we
> > > > > > found couple of issues with Hive-2 and sentry 2.0 integration
> using
> > > > > auth-v2
> > > > > > interface. There were couple of fixes that are needed from Hive
> > side,
> > > > > some
> > > > > > of them are already fixed in Hive 3.0 and some are yet to be
> fixed.
> > > > > Bottom
> > > > > > line is that we need some help from Hive community in fixing some
> > > > issues
> > > > > > and back-porting them to some Hive-2.x release before we actually
> > try
> > > > to
> > > > > > continue testing it with new Hive-2.x release. This could take
> > quite
> > > > some
> > > > > > time
> > > > > >
> > > > > > *Integrating with Solr-7: *We happened to talk to contributors of
> > > this
> > > > > > feature. They might some time to actually get it ready.
> > > > > >
> > > > > > These outstanding work need not delay the release of sentry-HA
> > which
> > > is
> > > > > > well tested locally at cloudera. This feature is something that
> the
> > > > > > community is waiting for quite some time. Why don't we release
> > > current
> > > > > > master as sentry 1.9 release?
> > > > > >
> > > > > > Sentry 1.9.0 if released, would below features.
> > > > > >
> > > > > >1. Sentry-HA
> > > > > >2. Integration with Hive 2.0 using auth-V1.
> > > > > >
> > > > > > Seeking inputs from every one in this regard.
> > > > > >
> > > > > >
> > > > > > -Kalyan
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Colm O hEigeartaigh
> > > >
> > > > Talend Community Coder
> > > > http://coders.talend.com
> > > >
> > >
> >
>

Re: Review Request 62979: SENTRY-1985: Sentry should log in stdout when it is ready to serve requests

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 16, 2017, 3:35 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62979/
> ---
> 
> (Updated Oct. 16, 2017, 3:35 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1985
> https://issues.apache.org/jira/browse/sentry-1985
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Display when Sentry is ready to serve on the console output.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  31fd4597dad6b02bb5d8b9f944a488f6f78d7d79 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  d2a4c2dc2607f5095855e9769bb6597893c0aa49 
> 
> 
> Diff: https://reviews.apache.org/r/62979/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 63047: SENTRY-1994: Bump Shiro dependency version to 1.4.0

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 16, 2017, 9:11 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63047/
> ---
> 
> (Updated Oct. 16, 2017, 9:11 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-1994
> https://issues.apache.org/jira/browse/sentry-1994
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bumpe shiro version to bring security fix reported on 
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=%20CVE-2016-4437
> 
> 
> Diffs
> -
> 
>   pom.xml 370b899b372186305c84337a6a8b957e1faa861d 
> 
> 
> Diff: https://reviews.apache.org/r/63047/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 63048: SENTRY-1995: Bump Derby dependency version to 10.13.1.1

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 16, 2017, 9:16 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63048/
> ---
> 
> (Updated Oct. 16, 2017, 9:16 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-1995
> https://issues.apache.org/jira/browse/sentry-1995
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bumpe derby version to 10.13.1.1 that has a security bugfix 
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=%20CVE-2015-1832
> 
> 
> Diffs
> -
> 
>   pom.xml 370b899b372186305c84337a6a8b957e1faa861d 
> 
> 
> Diff: https://reviews.apache.org/r/63048/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62942: Bump the minimum java version to 8

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 13, 2017, 6:30 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62942/
> ---
> 
> (Updated Oct. 13, 2017, 6:30 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colm O hEigeartaigh, and 
> kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1893
> https://issues.apache.org/jira/browse/sentry-1893
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump the version of Java to JDK8 as minmum.
> 
> 
> Diffs
> -
> 
>   pom.xml a02891223417eccd3976d97d247c8fcb626afa78 
> 
> 
> Diff: https://reviews.apache.org/r/62942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62990: SENTRY-1988: Bump slf4j version from 1.6.0 to 1.7.25 (latest version)

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 13, 2017, 9:03 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62990/
> ---
> 
> (Updated Oct. 13, 2017, 9:03 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: sentry-1988
> https://issues.apache.org/jira/browse/sentry-1988
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bumped version to 1.7.25
> 
> 
> Diffs
> -
> 
>   pom.xml a02891223417eccd3976d97d247c8fcb626afa78 
> 
> 
> Diff: https://reviews.apache.org/r/62990/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62942: Bump the minimum java version to 8

[Alexander Kolbasov]

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



You also need to update version for com.google.errorprone

- Alexander Kolbasov


On Oct. 12, 2017, 6:16 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62942/
> ---
> 
> (Updated Oct. 12, 2017, 6:16 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Colm O hEigeartaigh, and 
> kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1893
> https://issues.apache.org/jira/browse/sentry-1893
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump the version of Java to JDK8 as minmum.
> 
> 
> Diffs
> -
> 
>   pom.xml aa422a042e1496d429180122bde14bb4edb26906 
> 
> 
> Diff: https://reviews.apache.org/r/62942/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62950: SENTRY-1981: Bump codehale metrics version to latest 3.0.2 version

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 12, 2017, 8:49 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62950/
> ---
> 
> (Updated Oct. 12, 2017, 8:49 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1981
> https://issues.apache.org/jira/browse/sentry-1981
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump codehale metrics to 3.0.2 version.
> 
> 
> Diffs
> -
> 
>   pom.xml aa422a042e1496d429180122bde14bb4edb26906 
> 
> 
> Diff: https://reviews.apache.org/r/62950/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62953: SENTRY-1983: Several commit/rollback errors happen in oracle12c-r1 due to current isolation level

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 12, 2017, 10:03 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62953/
> ---
> 
> (Updated Oct. 12, 2017, 10:03 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Bugs: sentry-1983
> https://issues.apache.org/jira/browse/sentry-1983
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Changes the isolation level from serializable to read-committed only for 
> ORacle.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  9a37ac8fe572a98b3fb4cba127bcb1716bd8f017 
> 
> 
> Diff: https://reviews.apache.org/r/62953/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 63026: SENTRY-1979 - Consolidate code for converting Hive privilege objects to Strings

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Oct. 16, 2017, 11:33 a.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63026/
> ---
> 
> (Updated Oct. 16, 2017, 11:33 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1979
> https://issues.apache.org/jira/browse/SENTRY-1979
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This task is to consolidate the code for converting Hive privileges objects 
> to Strings. We have some duplicate code that is used by the shell that could 
> be removed.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java
>  51ee9efc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java
>  e3d06a97 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java
>  5f3e9fbc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java
>  fe6aca5d 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java
>  81059c5f 
> 
> 
> Diff: https://reviews.apache.org/r/63026/diff/1/
> 
> 
> Testing
> ---
> 
> Tested the Hive Shell.
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Review Request 62983: SENTRY-1986: Fix NPE on createGrantTask from SentryHiveAuthorizationTaskFactoryImpl.java

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
Line 142 (original), 142 (patched)
<https://reviews.apache.org/r/62983/#comment265049>

This means that privilegeObj can never be null. But this also shows that 
dump() is called for no reason all the time here and it is potentially 
expensive and should be removed.


- Alexander Kolbasov


On Oct. 13, 2017, 6:10 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62983/
> ---
> 
> (Updated Oct. 13, 2017, 6:10 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-1986
> https://issues.apache.org/jira/browse/sentry-1986
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Fix NPE
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
>  ceb3b17714d5dfc4c6186b5f9cf536d6ddbb662b 
> 
> 
> Diff: https://reviews.apache.org/r/62983/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Proposal to release Sentry 2.0.0

[Alexander Kolbasov]

Kalyan,

Thank you for pushing forward 2.0 release!

> On Oct 13, 2017, at 7:20 AM, Kalyan Kumar Kalvagadda  
> wrote:
> 
> Hello all,
> 
> We need to release sentry HA functionality so that community can start
> using it. In this regard I proposed to have a sentry 1.9.0 release as there
> were some outstanding issues integrating with Hive. Community was not
> positive on this proposal for various reasons.

It would be great if you can summarize these reasons here for future reference.

> 
> With recent findings we think we don't have to wait for Hive fixes. With
> the changes that are planned for below listed Jira's sentry would use a
> combination of Semantic Hooks and AuthV2 interface to integrate with HIVE
> 2.0.

In your earlier email regarding 1.9 release you also mention issues with moving 
up to Java 8 and some issues with Solr-7 integration. Does this mean that you 
have a better idea of how to deal with these issues now?


> 
>   1. SENTRY-1978  -Move
>   the hive-authz2 grant/revoke implementation into the sentry-binding-hive
>   module

This looks like “refactoring change” so it doesn’t actually change any existing 
functionality - right?

> 
> 
>   1. SENTRY-1980 - Move
>   the hive-authz2 HMS client filtering implementation into the
>   sentry-binding-hive module.

Same here - this seems to be a refactoring change which doesn’t affect any 
existing functionality.

I think you are referring to some planned follow-up work to actually solve the 
authorization problem for Hive 2 - right?

> 
> 
> 
> I have created a umbrella jira for releasing Sentry 2.0 .
> 
>   1. SENTRY-1982  -Release
>   sentry 2.0.0 upstream
> 
> 
> I have linked issues that are blocking the release. If you see any issue
> that is blocking please attach it to this jira. That way we can fix them
> and clear all the road blocks for releasing SENTYR 2.0.0

What is your impression once you looked at these issues - do you think that you 
should be able to fix majority of these or do you think that these ca nbe 
simply moved out of the release?

> 
> 
> -Kalyan

- Alex

Re: Review Request 62195: SENTRY-1934: SQL Index name is too long for Oracle 11.2

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 8, 2017, 9:59 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62195/
> ---
> 
> (Updated Sept. 8, 2017, 9:59 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> make the index shorter than 30 bytes
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  88d7378 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql
>  49b7959 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
>  0bda5cd 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
>  8dd8316 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql
>  2610d6c 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 
> 1661412 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 
> 1b95622 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> f033895 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 
> 1b5cd04 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql
>  d6d222e 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.8.0-to-2.0.0.sql
>  6340520 
> 
> 
> Diff: https://reviews.apache.org/r/62195/diff/1/
> 
> 
> Testing
> ---
> 
> test on Oracle DB 11.2. The changed index works
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 62192: SENTRY-1932: Improve logging for HMSPath

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 8, 2017, 10:37 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62192/
> ---
> 
> (Updated Sept. 8, 2017, 10:37 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently HMSPath logs too much information when it skips paths which are 
> outside the prefix:
> 
> //Begin of log snippet
> 2017-09-07 15:29:39,041 INFO org.apache.sentry.hdfs.HMSPaths: Path outside 
> prefix
> 2017-09-07 15:29:39,040 INFO org.apache.sentry.hdfs.HMSPaths: Skipping to 
> create authzObjPath as it is outside of prefix. authObj = l5_foo.customer_v1 
> pathElements=[data, blah, l5_blah_blah, CUSTOMER_V1, eap_part_no=12345]
> //End of log snippet
> 
> This happens when Sentry sends a full snapshot and when NameNode receives 
> delta snapshots.
> Probably we should log information more clearly and log at DEBUG level.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  5a982637d 
> 
> 
> Diff: https://reviews.apache.org/r/62192/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 62194: SENTRY-1933: hive-authz2 build fails because SentryJSONAlterPartitionMessage is not compatible

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 8, 2017, 9:26 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62194/
> ---
> 
> (Updated Sept. 8, 2017, 9:26 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Na Li.
> 
> 
> Bugs: sentry-1933
> https://issues.apache.org/jira/browse/sentry-1933
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch adds a new constructor visible for testing only to allow the 
> hive-authz1 and hive-authz2 tests to compile.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-follower-v2/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
>  64c15158b83d440577bcd2b717253ef9266d9b10 
>   
> sentry-binding/sentry-binding-hive-follower/src/main/java/org/apache/sentry/binding/metastore/messaging/json/SentryJSONAlterPartitionMessage.java
>  b29d727561ab0d2645c3c02fb118ca5c35508575 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  a8fcbf8f36482661bb40adf481105af4e4438689 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  e9ae6a1a503192c7fe6e6076913fdbc41957e56c 
> 
> 
> Diff: https://reviews.apache.org/r/62194/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 61986: SENTRY-1907 Potential memory optimization when handling big full snapshots.

[Alexander Kolbasov]

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

(Updated Aug. 30, 2017, 5:58 a.m.)


Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Addressed code review comment from Misha.


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


Repository: sentry


Description
---

SENTRY-1907 Potential memory optimization when handling big full snapshots.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 f1e67ca6e343e4e5dd0c3377c4c6beb19544e3eb 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Review Request 61988: SENTRY-1903 TransactionManager shows retried transactions starting from 0

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, and 
Vamsee Yarlagadda.


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


Repository: sentry


Description
---

SENTRY-1903 TransactionManager shows retried transactions starting from 0


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
 0a9f3a7cfd2790fa9b0a9b15323ea4a4c53184c3 


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


Testing
---


Thanks,

Alexander Kolbasov

Review Request 61986: SENTRY-1907 Potential memory optimization when handling big full snapshots.

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1907 Potential memory optimization when handling big full snapshots.


Diffs
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 f1e67ca6e343e4e5dd0c3377c4c6beb19544e3eb 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61983: SENTRY-1906: Sentry clients to retry connections to server with delay to avoid failing fast

[Alexander Kolbasov]

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



My concern with this change is that we will delay normal client failover by 3 
seconds. Can we initially retry without delay and then delay if it failed?


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
Lines 157 (patched)
<https://reviews.apache.org/r/61983/#comment260233>

How are you handling InterruptedException here?


- Alexander Kolbasov


On Aug. 30, 2017, 1:29 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61983/
> ---
> 
> (Updated Aug. 30, 2017, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client logic supports retry of connections to server. But it should be 
> complemented with sufficient retry interval otherwise they fail fast without 
> trying for a decent time.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  d400f8999d74cbc66c19a739a05d10a201635c5b 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  9fd401316f5184a80d737c463e03291bdebb2287 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  358d2824b7ce55a89945d321bf1a27fdbdd71e62 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  1724e7f9610e1e65cc24bd690d73530f6fb14049 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  45522df0d5803f525c29bfa9b06fde9e3c7e5939 
> 
> 
> Diff: https://reviews.apache.org/r/61983/diff/1/
> 
> 
> Testing
> ---
> 
> Sentry unit test run in progress.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Re: Review Request 61983: SENTRY-1906: Sentry clients to retry connections to server with delay to avoid failing fast

[Alexander Kolbasov]

> On Aug. 30, 2017, 5:36 p.m., Alexander Kolbasov wrote:
> > My concern with this change is that we will delay normal client failover by 
> > 3 seconds. Can we initially retry without delay and then delay if it failed?
> 
> Vamsee Yarlagadda wrote:
> This will not happen. The actual logic of client trying all the servers 
> is under SentryTransportPool. Every retry under RetryClientConnectionHandler 
> attempts connecting to all servers exhaustively (without any delay). So we 
> don't have to worry about adding delays for quick failover.

Oh, ok. no problem then.


- Alexander


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


On Aug. 30, 2017, 1:29 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61983/
> ---
> 
> (Updated Aug. 30, 2017, 1:29 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client logic supports retry of connections to server. But it should be 
> complemented with sufficient retry interval otherwise they fail fast without 
> trying for a decent time.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  d400f8999d74cbc66c19a739a05d10a201635c5b 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  9fd401316f5184a80d737c463e03291bdebb2287 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  358d2824b7ce55a89945d321bf1a27fdbdd71e62 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  1724e7f9610e1e65cc24bd690d73530f6fb14049 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  45522df0d5803f525c29bfa9b06fde9e3c7e5939 
> 
> 
> Diff: https://reviews.apache.org/r/61983/diff/1/
> 
> 
> Testing
> ---
> 
> Sentry unit test run in progress.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Re: Review Request 61973: SENTRY-1888: Sentry might not fetch all HMS duplicated events IDs when requested

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4135 (patched)
<https://reviews.apache.org/r/61973/#comment260300>

We can also set pm.setDetachAllOnCommit(false) and another setting that 
avoids loading data on close.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4136 (patched)
<https://reviews.apache.org/r/61973/#comment260299>

hash is a primary key, so we can set unique.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 63 (original), 64 (patched)
<https://reviews.apache.org/r/61973/#comment260288>

This should be HiveCOnnectionFactory as well



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 247 (original), 251 (patched)
<https://reviews.apache.org/r/61973/#comment260289>

Style - is it preferrable to use // style comments to simplify commenting 
out block of code with /* comments?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 248 (original), 252 (patched)
<https://reviews.apache.org/r/61973/#comment260290>

wording: number of notifications can't have agap. sequence of notifications 
can.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 254 (original), 258 (patched)
<https://reviews.apache.org/r/61973/#comment260291>

Wording here. May be you can reword it here. E.g.

HMS notifications may contain both gaps in the sequence and duplicates (the 
same ID repeated more then once for different events). Duplicates do not 
necessarily come one after another, so the sequence {1,2,3,2} is possible



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 260 (original)
<https://reviews.apache.org/r/61973/#comment260292>

So you decided not to log anything in this case?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 41 (patched)
<https://reviews.apache.org/r/61973/#comment260295>

can be final



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 43 (patched)
<https://reviews.apache.org/r/61973/#comment260296>

can be final



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 47 (patched)
<https://reviews.apache.org/r/61973/#comment260297>

can use HashSet<>



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 49 (patched)
<https://reviews.apache.org/r/61973/#comment260298>

this and all other methods can be package-private



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
Lines 127 (patched)
<https://reviews.apache.org/r/61973/#comment260315>

If I am reading this code correctly, the following will happen:

1) Every time we request IDs starting from last seen ID, so normally the 
first ID in the sequence is always a duplicate
2) This first ID in the sequence isn't in the cache yet, so we go to the 
sentryStore and fetch the hash and compare.

This means that we always go to sentryStore for the frist element of the 
batch which means every time we call fetchNotifications().

This is not very bad, but if we store the sha1 hash of the last event in 
the cache we don't have to go to sentryStore at all under normal circumstances


- Alexander Kolbasov


On Aug. 29, 2017, 6:41 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61973/
> -------
> 
> (Updated Aug. 29, 2017, 6:41 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Na Li.
> 
> 
> Bugs: sentry-1888
> https://issues.apache.org/jira/browse/sentry-1888
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Approach:
> The proposed solution will be to request notifications from the last ID seen 
> again. This way we could bring current duplicates and apply them on Sentry. 
> We have the risk to miss duplicates that were committed much time later, but 
> we cannot trust on those duplicates as they will not know the order of the 
&

Re: Review Request 61983: SENTRY-1906: Sentry clients to retry connections to server with delay to avoid failing fast

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Aug. 30, 2017, 7:33 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61983/
> ---
> 
> (Updated Aug. 30, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar 
> kalvagadda, and Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client logic supports retry of connections to server. But it should be 
> complemented with sufficient retry interval otherwise they fail fast without 
> trying for a decent time.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  d400f8999d74cbc66c19a739a05d10a201635c5b 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  9fd401316f5184a80d737c463e03291bdebb2287 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  358d2824b7ce55a89945d321bf1a27fdbdd71e62 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  1724e7f9610e1e65cc24bd690d73530f6fb14049 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  45522df0d5803f525c29bfa9b06fde9e3c7e5939 
> 
> 
> Diff: https://reviews.apache.org/r/61983/diff/2/
> 
> 
> Testing
> ---
> 
> Sentry unit test run in progress.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Review Request 62007: SENTRY-1909 Improvements for memory usage when full path snapshot is sent from Sentry to NN

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1909 Improvements for memory usage when full path snapshot is sent from 
Sentry to NN


Diffs
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 898c7bec2e35e6f1424478666282ba78222da709 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 20b3e108cc976207a3809bc6a214a34e10788200 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 e403d7cca5dca5c39785490f92e562dc9a3b4daf 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
 fee5279a7cd3d60df4e60a5f25d2e8604c37d04b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java
 409a557fe89d85df888c1f16b3f5aacdd9aa96b0 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 593b92f96b47f959266280bce3d0809f6a80c362 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 760a2b55e98ac1fb305666cbbedfba11d26f2fcc 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 2cd18ea9d2ca40b5f46aaafde532fb3bf9769c07 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4a8fb953ce486e1aeb1042884de56872b5539cd0 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 3da6a4e998d36cada7e9ea77285b11f07cea5f25 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
 9d7bddd339513180e627286d8a749b7814c824f2 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 7deccb064fd75b39af779796d9e95caf9c718b32 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
 f56384a99e128b3e9880cbc5692107d61f2f500f 


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


Testing
---


Thanks,

Alexander Kolbasov

Blog entry for Sentry 1.8 release

[Alexander Kolbasov]

Sergio - would you like to write a blog entry for the 1.8 release? I think
others can help by pointing out major things that were added in 1.8 that
they know about.

- Alex.

Re: Review Request 62221: SENTRY-1938: Sentry logs to provide more relevant information

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 11, 2017, 6:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62221/
> ---
> 
> (Updated Sept. 11, 2017, 6:56 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Na Li, Sergio 
> Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry Supportability improvement.
> 
> 1. HMSFollower 
> * Print confirmation message (at INFO level) once full snapshot is persisted 
> in the DB.
> * Print the message that HMSFollower is completely ready (after the initial 
> pass of HMSFollower is done)
> 
> 2. DBUpdateForwarder 
> * Every log message should explicitly mention which type of events is it 
> referring to (PERM or PATH) otherwise there is no way for us to differentiate 
> between calls.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  1318082d3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  b600487dc 
> 
> 
> Diff: https://reviews.apache.org/r/62221/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 62221: SENTRY-1938: Sentry logs to provide more relevant information

[Alexander Kolbasov]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 143 (patched)
<https://reviews.apache.org/r/62221/#comment261309>

This is the most common case meaning that we'll get this log every 1/2 
seconds.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 313 (original), 314 (patched)
<https://reviews.apache.org/r/62221/#comment261310>

Here it is not persisted, so the message is wrong.


- Alexander Kolbasov


On Sept. 11, 2017, 3:32 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62221/
> ---
> 
> (Updated Sept. 11, 2017, 3:32 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Na Li, Sergio 
> Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry Supportability improvement.
> 
> 1. HMSFollower 
> * Print confirmation message (at INFO level) once full snapshot is persisted 
> in the DB.
> * Print the message that HMSFollower is completely ready (after the initial 
> pass of HMSFollower is done)
> 
> 2. DBUpdateForwarder 
> * Every log message should explicitly mention which type of events is it 
> referring to (PERM or PATH) otherwise there is no way for us to differentiate 
> between calls.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  1318082d3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  b600487dc 
> 
> 
> Diff: https://reviews.apache.org/r/62221/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 62219: SENTRY-1941: Add log4j2.properties file to sentry-hive-tests-v2

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 11, 2017, 2:52 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62219/
> ---
> 
> (Updated Sept. 11, 2017, 2:52 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Vamsee Yarlagadda.
> 
> 
> Bugs: sentry-1941
> https://issues.apache.org/jira/browse/sentry-1941
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add this new file so that sentry tests for hive 2.x display log information 
> on the test logs.
> 
> 
> Diffs
> -
> 
>   sentry-tests/sentry-tests-hive-v2/src/test/resources/log4j2.properties 
> e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
> 
> 
> Diff: https://reviews.apache.org/r/62219/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 62075: SENTRY-1917: Sentry should work around HIVE-16994

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
Vamsee Yarlagadda, and Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1917: Sentry should work around HIVE-16994


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
 d7f43dd939424f1efdd0d2501d5bdaefd346a8e1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 d62196f7c869aa088db4f9d110a9c4d94387ac14 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClient.java
 86ff47e00fcad78f7f2c1c8f2dce027fb503daa6 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveConnectionFactory.java
 62542c34271decb2aacf087c5450dff80d970b19 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HivePoolConnectionFactory.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 77634cf25fbffaab51556bdde35784ec4fef3f28 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 6ec163b1d802810287a55bc5cd98409bc73e8ba8 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
 d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
 589acbed12855ff09309a04c9214f8daf87ea1de 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
 38668ca88f155e0fa2f3beb4db32b18e953e58b6 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62096: SENTRY-1919: Sentry should prevent two snapshots from being sent to HDFS

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
Lines 46 (patched)
<https://reviews.apache.org/r/62096/#comment260758>

I think it is a bit cleaner to change the name and reverse the value, 
something like

pathRetrieverIsBusy = newAtomicBoolean(true)

This will express the meaning better IMO.


- Alexander Kolbasov


On Sept. 5, 2017, 9:11 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62096/
> ---
> 
> (Updated Sept. 5, 2017, 9:11 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, Na Li, and 
> Vamsee Yarlagadda.
> 
> 
> Bugs: sentry-1919
> https://issues.apache.org/jira/browse/sentry-1919
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch uses an atomic boolean flag that prevents multiple HDFS requests to 
> get the paths updates (either deltas or full images) by returning an empty 
> paths updates list if the flag is set as not available.
> 
> The reason to prevent for deltas as well is because the code change was much 
> easier to do in once place (SentryHDFSServiceProcessor). We shouldn't get any 
> issues if we do it this way (unless the reviewer thinks otherwise).
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java
>  6221f3d01b2d86ec257bcec290c9b3b0527a6e34 
> 
> 
> Diff: https://reviews.apache.org/r/62096/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 62101: SENTRY-1916 Sentry should not store paths outside of the prefix

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, and 
Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1916 Sentry should not store paths outside of the prefix


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 55092eec04d86b64aa91e30e76bb63c40b2a375b 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 551f184c35c8c3bf4e4b49aa540a55b016bb9a32 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 d62196f7c869aa088db4f9d110a9c4d94387ac14 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 25019708411c66c43700f7219b4059dd31af80a3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 d4feb380fa0f3bd5f237609a107295a2d23eae3b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
 713fc3d135fe1a6a0c0d30ef9610540d350aa782 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathValidator.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 6ec163b1d802810287a55bc5cd98409bc73e8ba8 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
 589acbed12855ff09309a04c9214f8daf87ea1de 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 a8fcbf8f36482661bb40adf481105af4e4438689 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java
 00f9b3959b7ccf9d046c422a007ff375eddba076 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
 38668ca88f155e0fa2f3beb4db32b18e953e58b6 
  
sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 acf1cf5e2a9ba1cfaef9ccc404e56791c10b4be9 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 63e718cd403ee3bbfa62ab572ac678b418b82cc7 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
 a8d70c11c879828d16f44df2168853a5b709c8f7 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

[Alexander Kolbasov]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 81 (patched)
<https://reviews.apache.org/r/62107/#comment260784>

Are you sure you get your boolean logic right?

Also, if we are not going to retrieve full update, why bother calling 
`imageRetriever.getLatestImageID();` ?

Also it would be nice to log a message telling that we are rather busy at 
the moment, call back later, please



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 115 (patched)
<https://reviews.apache.org/r/62107/#comment260785>

Why are we doing this the second time? ANyway, all the comments above apply 
here.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 106 (patched)
<https://reviews.apache.org/r/62107/#comment260783>

I think this can be package-private



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 297 (patched)
<https://reviews.apache.org/r/62107/#comment260779>

What if this was already set?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 289 (original), 300 (patched)
<https://reviews.apache.org/r/62107/#comment260780>

Here we return, leaving the value loadingFullSnapshot set to true forever.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 294 (original), 305 (patched)
<https://reviews.apache.org/r/62107/#comment260781>

Ditto



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 220 (patched)
<https://reviews.apache.org/r/62107/#comment260782>

This can be simplified to

return hmsFollower != null && hmsFollower.isLoadingFullSnapshot();

Regarding the null == hmsFollower vs hmsFollower == null - we had this 
discussion earlier. 

In Java you can't say

hmsFollower != null && hmsFollower.isLoadingFullSnapshot();

so you are not getting any advantage in the extra checks. And for a human 
parser, reading null == something is much worse then something == null. We are 
not using this reverse notation in most other places and I suggest that we keep 
it this way.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java
Line 22 (original), 21 (patched)
<https://reviews.apache.org/r/62107/#comment260786>

How do these changes relate to the issue you are fixing? Are you trying to 
sneak in some other change?


- Alexander Kolbasov


On Sept. 6, 2017, 12:42 a.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> ---
> 
> (Updated Sept. 6, 2017, 12:42 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio 
> Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
> https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added fix to limit sending back of full snapshot when the HMSFollower is 
> pulling a full snapshot
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java
>  1685702c6dbc9715c8885a29a80bc68509550f0b 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/1/
> 
> 
> Testing
> ---
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62146: SENTRY-1929 When full HMS snapshot is created all higher notifications should be purged

[Alexander Kolbasov]

> On Sept. 7, 2017, 4:03 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 402 (patched)
> > <https://reviews.apache.org/r/62146/diff/3/?file=1817058#file1817058line402>
> >
> > I don't see CounterWait has the function reset(). did you forget to 
> > upload change of this file?

It is there if you click on CounterWait.java file. It is included in this 
review.


- Alexander


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


On Sept. 7, 2017, 8:06 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62146/
> ---
> 
> (Updated Sept. 7, 2017, 8:06 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1929
> https://issues.apache.org/jira/browse/SENTRY-1929
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1929 When full HMS snapshot is created all higher notifications should 
> be purged
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1ef7dcc6e6615da8ddd3fae18f1026dc0a8505e6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
>  9c9bb69e768a1e648bb16e8c7efaa6ca6edea797 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  93afb614d0916c60ca416f8904331e78dd1b85d2 
> 
> 
> Diff: https://reviews.apache.org/r/62146/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Review Request 62146: SENTRY-1929 When full HMS snapshot is created all higher notifications should be purged

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
Vamsee Yarlagadda, and Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1929 When full HMS snapshot is created all higher notifications should 
be purged


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1ef7dcc6e6615da8ddd3fae18f1026dc0a8505e6 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62112: SENTRY-1915 Sentry is doing a lot of work to convert list of paths to HMSPaths structure

[Alexander Kolbasov]

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

(Updated Sept. 6, 2017, 6:48 p.m.)


Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Added simple unit test


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


Repository: sentry


Description
---

SENTRY-1915 Sentry is doing a lot of work to convert list of paths to HMSPaths 
structure


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 719c1acc18ce627f5228efb0d9cf47a7b5810a1b 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
 cbdc7ecdbeee0825b57b0c4408e0600f601a166c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 1762ee5fd21bb49f6593c2f0a0cc5810bfc4b8dd 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 1bdebb13b4cf981f0d69d93574a3ba317d9822ba 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 4652dc9e0a35a20780418d8e388595198a8f9ccd 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 83340342851598caf86f7c9e8e4d79f0e6027b20 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 35417b76970b5b101a20c12ba96fdae763681aec 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62177: SENTRY-1931: NameNode only gets full snapshot once

[Alexander Kolbasov]

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


Ship it!




Ship It!


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
Lines 89 (patched)
<https://reviews.apache.org/r/62177/#comment261099>

Nit: why do we need to set it if the value is the same? So shouldn't it be 
`<=` ?


- Alexander Kolbasov


On Sept. 7, 2017, 10:23 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62177/
> ---
> 
> (Updated Sept. 7, 2017, 10:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
> 
> 
> Bugs: sentry-1931
> https://issues.apache.org/jira/browse/sentry-1931
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch sets the image ID to the PathsUpdate object so the HDFS can get the 
> updated information.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java
>  450395079970904398517a9e318d54848e06003b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  ccc10ef77635aa951569ac921409f9ded44d8cba 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
>  7aa60a318b1138000ac17d0d40ca99b1cb859559 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java
>  795aac7b3919b4ea1a7bcc1d385128639c91217d 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  819b6b2932bcad3e4e52e282766ad16bb5408d03 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java
>  fda7455d284002ced547c9e4b695fc8efb4ed207 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
>  df6a0b0afd1e0d1e99c4ce3d7b40e34a2117ad41 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java
>  63b8caf2faa95bdf0b63affbcac96653765af2ea 
> 
> 
> Diff: https://reviews.apache.org/r/62177/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62112: SENTRY-1915 Sentry is doing a lot of work to convert list of paths to HMSPaths structure

[Alexander Kolbasov]

> On Sept. 6, 2017, 7:09 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2760 (patched)
> > <https://reviews.apache.org/r/62112/diff/1/?file=1816137#file1816137line2761>
> >
> > Do we need to intern the strings to reduce memory usage? similar like 
> > below?
> > 
> > +  List paths = new ArrayList<>(pathComponents.length);
> > +  for (String pathElement: pathComponents) {
> > +paths.add(pathElement.intern());
> > +  }

In this case interning isn't going to help because we are storing all paths as 
a tree, so this should take care of the duplicate strings. it is unlikely that 
we have duplicate strings at different levels of a tree.


- Alexander


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


On Sept. 6, 2017, 6:48 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62112/
> ---
> 
> (Updated Sept. 6, 2017, 6:48 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1915
> https://issues.apache.org/jira/browse/SENTRY-1915
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1915 Sentry is doing a lot of work to convert list of paths to 
> HMSPaths structure
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  719c1acc18ce627f5228efb0d9cf47a7b5810a1b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  cbdc7ecdbeee0825b57b0c4408e0600f601a166c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  1762ee5fd21bb49f6593c2f0a0cc5810bfc4b8dd 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  1bdebb13b4cf981f0d69d93574a3ba317d9822ba 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  83340342851598caf86f7c9e8e4d79f0e6027b20 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  35417b76970b5b101a20c12ba96fdae763681aec 
> 
> 
> Diff: https://reviews.apache.org/r/62112/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62136: SENTRY-1928: HMSFollower should close HMS connections when an error to HMS occurs

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 191 (original), 191 (patched)
<https://reviews.apache.org/r/62136/#comment260957>

Since we are not doing anything fancy now, should we just remove this and 
have a simple catch-all?


- Alexander Kolbasov


On Sept. 6, 2017, 10:50 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62136/
> ---
> 
> (Updated Sept. 6, 2017, 10:50 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda.
> 
> 
> Bugs: sentry-1928
> https://issues.apache.org/jira/browse/sentry-1928
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch is closing the HMS connections on the HiveNotificationFetcher if 
> any Exception occurrs. The HMSFollower has also code to close the connection 
> on any Exception, but I do it for legacy reasons as I think it is not 
> required anymore. The HMSFollower needs some refactoring, though.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
>  4d329921bb4bc540992084687726155cb0373f0f 
> 
> 
> Diff: https://reviews.apache.org/r/62136/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

> On Sept. 13, 2017, 12:39 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 1774 (patched)
> > <https://reviews.apache.org/r/62261/diff/1/?file=1820339#file1820339line1800>
> >
> > Looks like in the old code, 'groupName == null' was supported, and 
> > getAllRoles(pm) was returned in that case. I don't think it happening here 
> > anymore?

We never call it with groupName == null


- Alexander


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


On Sept. 12, 2017, 11:26 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62261/
> ---
> 
> (Updated Sept. 12, 2017, 11:26 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1937
> https://issues.apache.org/jira/browse/SENTRY-1937
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1937 Optimize performance for listing sentry roles by group name
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  01a7c830f548d41062fb3bbd0a81f71514585aa5 
> 
> 
> Diff: https://reviews.apache.org/r/62261/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

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

(Updated Sept. 13, 2017, 1:32 a.m.)


Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, and Vamsee Yarlagadda.


Changes
---

Implemented the case with group being null


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


Repository: sentry


Description
---

SENTRY-1937 Optimize performance for listing sentry roles by group name


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 01a7c830f548d41062fb3bbd0a81f71514585aa5 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

> On Sept. 13, 2017, 12:39 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 1774 (patched)
> > <https://reviews.apache.org/r/62261/diff/1/?file=1820339#file1820339line1800>
> >
> > Looks like in the old code, 'groupName == null' was supported, and 
> > getAllRoles(pm) was returned in that case. I don't think it happening here 
> > anymore?
> 
> Alexander Kolbasov wrote:
> We never call it with groupName == null

Actually tests do, so dealing with this case as well.


- Alexander


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


On Sept. 12, 2017, 11:26 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62261/
> ---
> 
> (Updated Sept. 12, 2017, 11:26 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1937
> https://issues.apache.org/jira/browse/SENTRY-1937
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1937 Optimize performance for listing sentry roles by group name
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  01a7c830f548d41062fb3bbd0a81f71514585aa5 
> 
> 
> Diff: https://reviews.apache.org/r/62261/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

> On Sept. 13, 2017, 12:39 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 1774 (patched)
> > <https://reviews.apache.org/r/62261/diff/1/?file=1820339#file1820339line1800>
> >
> > Looks like in the old code, 'groupName == null' was supported, and 
> > getAllRoles(pm) was returned in that case. I don't think it happening here 
> > anymore?
> 
> Alexander Kolbasov wrote:
> We never call it with groupName == null
> 
> Alexander Kolbasov wrote:
> Actually tests do, so dealing with this case as well.

As it turns out, the case of group == null is the main case used at least by 
Impala! I am seeing 20-30% improvement in this case as well.


- Alexander


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


On Sept. 13, 2017, 3:49 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62261/
> ---
> 
> (Updated Sept. 13, 2017, 3:49 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1937
> https://issues.apache.org/jira/browse/SENTRY-1937
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1937 Optimize performance for listing sentry roles by group name
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  01a7c830f548d41062fb3bbd0a81f71514585aa5 
> 
> 
> Diff: https://reviews.apache.org/r/62261/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, and Vamsee Yarlagadda.


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


Repository: sentry


Description
---

SENTRY-1937 Optimize performance for listing sentry roles by group name


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 01a7c830f548d41062fb3bbd0a81f71514585aa5 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62261: SENTRY-1937 Optimize performance for listing sentry roles by group name

[Alexander Kolbasov]

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

(Updated Sept. 13, 2017, 3:49 a.m.)


Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, and Vamsee Yarlagadda.


Changes
---

Added comments


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


Repository: sentry


Description
---

SENTRY-1937 Optimize performance for listing sentry roles by group name


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 01a7c830f548d41062fb3bbd0a81f71514585aa5 


Diff: https://reviews.apache.org/r/62261/diff/3/

Changes: https://reviews.apache.org/r/62261/diff/2-3/


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61793: SENTRY-1894: Update field size in package.jdo for dataNucleus to match size in sql

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Line 122 (original)
<https://reviews.apache.org/r/61793/#comment261165>

This change makes package.jdo inconsistent with sql scripts which do use 
these fields as keys and the goal was to make these consistent.


- Alexander Kolbasov


On Sept. 1, 2017, 9:53 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61793/
> ---
> 
> (Updated Sept. 1, 2017, 9:53 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> reduce field size `SERVER_NAME`,`DB_NAME`,`TABLE_NAME`,`COLUMN_NAME` in 
> `SENTRY_DB_PRIVILEGE` from 4000 to 128, and remove `URI` from being part of 
> the unique id
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  734ea7f 
> 
> 
> Diff: https://reviews.apache.org/r/61793/diff/4/
> 
> 
> Testing
> ---
> 
> run datanucleus schema tool. Without this change, it complains 
> "com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Specified key was 
> too long; max key length is 767 bytes. 
>  After those change, the datanucleus schema tool finishes successfully with 
> command "mvn datanucleus:schema-create -X -e"
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

[Alexander Kolbasov]

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



The approach is fine, the comments below are mostly about documentation about 
thread-safety of the state bank.


sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Line 105 (original), 107 (patched)
<https://reviews.apache.org/r/62107/#comment261153>

Sequence numbers do not share namespace with notification IDs, so this 
might confuse readers



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 281 (patched)
<https://reviews.apache.org/r/62107/#comment261156>

As I learned these asserts do not do anything at all since they are usually 
never enabled, so Preconditions are better - they will be triggered in testing.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java
Lines 17 (patched)
<https://reviews.apache.org/r/62107/#comment261157>

Please provide more documentation about these - what are these states and 
what is their semantics.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java
Lines 17 (patched)
<https://reviews.apache.org/r/62107/#comment261158>

Please provide more documentation about these states and their semantics. 
This is public API for these states.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java
Lines 18 (patched)
<https://reviews.apache.org/r/62107/#comment261159>

This is a publi cgeneric interface, please document what it is and how it 
should be used, as long as document interface getValue() method.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 27 (patched)
<https://reviews.apache.org/r/62107/#comment261160>

Please document API and thread safety.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 28 (patched)
<https://reviews.apache.org/r/62107/#comment261164>

make class final?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 31 (patched)
<https://reviews.apache.org/r/62107/#comment261161>

Someone may think that this class is thread safe, while it isn't. Is it 
worth making it completely thread-safe given that it is a generic utility class?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
Lines 53 (patched)
<https://reviews.apache.org/r/62107/#comment261162>

This part isn;t thread safe since it does non-atomic read/modify/write


- Alexander Kolbasov


On Sept. 7, 2017, 8:43 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> ---
> 
> (Updated Sept. 7, 2017, 8:43 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio 
> Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
> https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Check the state if the SentryService to see if its doing a full update and 
> delay the NN requests for a full update while thats going on.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c234eafb430b58bfd1e739847e83937a5e34d878 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerState.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceState.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryState.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryStateBank.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provi

Review Request 62229: SENTRY-1940 Sentry should time out threads waiting for notifications

[Alexander Kolbasov]

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

Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, and 
Vamsee Yarlagadda.


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


Repository: sentry


Description
---

SENTRY-1940 Sentry should time out threads waiting for notifications


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 a70a552a837263935ad84d7bd9a2aa507a2eb21e 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 cfd0e30c911f15a20b7d855d1bfa9a519142062a 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
 2c9e87a386a5e4f642a55bcd4636ae4c0752aa0d 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
 48aec1e80285b5a0184e771ccba35c762f813c59 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java
 e4846d9aea5ef9882fc655de9ec38c5795bde896 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62146: SENTRY-1929 When full HMS snapshot is created all higher notifications should be purged

[Alexander Kolbasov]

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

(Updated Sept. 7, 2017, 7:06 a.m.)


Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Addressed code review comments from Vamsee:

- Remove newer notifications when HDFS sync is disabled
- Reset waiting counter as well


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


Repository: sentry


Description
---

SENTRY-1929 When full HMS snapshot is created all higher notifications should 
be purged


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 1ef7dcc6e6615da8ddd3fae18f1026dc0a8505e6 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
 9c9bb69e768a1e648bb16e8c7efaa6ca6edea797 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 d4feb380fa0f3bd5f237609a107295a2d23eae3b 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62107: SENTRY-1918: NN snapshot should not be served while HMS snapshot is collected

[Alexander Kolbasov]

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



This is becoming quite big fix. I think it is better to split it into two 
independent parts:

1. Refactoring changes affecting the service factory
2. Actual changes for synchronization between components

That said, I think that it is possible to have much simpler fix for the 
original problem that doesn't require any of these changes. The idea is to 
create a helper singleton class where parties can register (by name) and say - 
hey, I am busy, hey, I am available - using a simple map from name to atomic 
value. This way we do not require all these plumbing changes (which may be 
useful for other purposes but that's another story).

- Alexander Kolbasov


On Sept. 6, 2017, 11:33 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62107/
> ---
> 
> (Updated Sept. 6, 2017, 11:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio 
> Pena, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1918
> https://issues.apache.org/jira/browse/SENTRY-1918
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Converted SentryService to a singleton and have HMSFollower setting a flag 
> when a full update is running.  Set the DBUpdateForwarder to only send back 
> full updates when then full update is not running.  Changed tests and added a 
> Helper to support the new singleton.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  5d744214e14d6c48194b3a0bf84d6e10070b020a 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  8fbc10048003ab4b8a38676e241ae0fd27d2392c 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  d4feb380fa0f3bd5f237609a107295a2d23eae3b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  d44abffc199cd46d3ab7d2230dfdb2075736a8f0 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java
>  1685702c6dbc9715c8885a29a80bc68509550f0b 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java
>  8959ad8ec77a1268a755faf451d99a81d87cb889 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java
>  3b3b30e0bdf65e774d854e8252339960afcdd306 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java
>  fe4164d4980738e8df6e93ee5f78e82e1d874ae1 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java
>  32e67b9f8efbbec12e93794f0ab00d5b8ed555b1 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
>  6895720f6e6d5f12ce468d7368e0d0ee9a0fae88 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java
>  b416ef81db21dfcf0157b2947c53721d5bcdf560 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  4cfb1f74cd90dbdb7c1aa5be07729e68353dd501 
>   sentry-tests/sentry-tests-kafka/pom.xml 
> 56a3ef10a9071929776cb7211bdb8ead921deace 
>   
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java
>  7c459993efb811b7ee11430f42791d1083f8d9df 
>   sentry-tests/sentry-tests-solr/pom.xml 
> c70476808688c80e1723d5e65e3b8cf6d1b64250 
>   
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java
>  ccea82ee8ee2b976f14bc6da46a84c764b3b96f9 
>   
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java
>  80f158a1fd626cdddeae9e2ee264f361d78bc2a7 
> 
> 
> Diff: https://reviews.apache.org/r/62107/diff/5/
> 
> 
> Testing
> ---
> 
> Unit Tests
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 62112: SENTRY-1915 Sentry is doing a lot of work to convert list of paths to HMSPaths structure

[Alexander Kolbasov]

> On Sept. 6, 2017, 3:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
> > Line 80 (original), 83 (patched)
> > <https://reviews.apache.org/r/62112/diff/1/?file=1816136#file1816136line83>
> >
> > If we return an empty PathsUpdate here, then we can enable this test.

We can do that but then the test itself is rather useless anyway


- Alexander


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


On Sept. 6, 2017, 6:48 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62112/
> ---
> 
> (Updated Sept. 6, 2017, 6:48 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1915
> https://issues.apache.org/jira/browse/SENTRY-1915
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1915 Sentry is doing a lot of work to convert list of paths to 
> HMSPaths structure
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  719c1acc18ce627f5228efb0d9cf47a7b5810a1b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  cbdc7ecdbeee0825b57b0c4408e0600f601a166c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  1762ee5fd21bb49f6593c2f0a0cc5810bfc4b8dd 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  1bdebb13b4cf981f0d69d93574a3ba317d9822ba 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  83340342851598caf86f7c9e8e4d79f0e6027b20 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  35417b76970b5b101a20c12ba96fdae763681aec 
> 
> 
> Diff: https://reviews.apache.org/r/62112/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62112: SENTRY-1915 Sentry is doing a lot of work to convert list of paths to HMSPaths structure

[Alexander Kolbasov]

> On Sept. 6, 2017, 3:46 p.m., Sergio Pena wrote:
> > Should we do the same change on PermImageRetriever as well? It is doing the 
> > same thing as PathImageRetriever.

Possibly, but I think it is much less important - it is usual to have lots and 
lots of partitions, but not as usual to have that many permissions.


> On Sept. 6, 2017, 3:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
> > Line 86 (original)
> > <https://reviews.apache.org/r/62112/diff/1/?file=1816134#file1816134line90>
> >
> > Should we keep these metrics here?

I'll take a look at adding these metrics back


> On Sept. 6, 2017, 3:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
> > Line 54 (original), 56 (patched)
> > <https://reviews.apache.org/r/62112/diff/1/?file=1816136#file1816136line56>
> >
> > If we return an empty PathsUpdate here, then we can enable this test.

I'll see what we can do with it - in general these tests should be seriously 
rewacked.


> On Sept. 6, 2017, 3:46 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2689 (patched)
> > <https://reviews.apache.org/r/62112/diff/1/?file=1816137#file1816137line2690>
> >
> > Any tests needed on TestSentryStore for this?

We need some test for this - my intention was to add these later.


- Alexander


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


On Sept. 6, 2017, 7:19 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62112/
> ---
> 
> (Updated Sept. 6, 2017, 7:19 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1915
> https://issues.apache.org/jira/browse/SENTRY-1915
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1915 Sentry is doing a lot of work to convert list of paths to 
> HMSPaths structure
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  719c1acc18ce627f5228efb0d9cf47a7b5810a1b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
>  cbdc7ecdbeee0825b57b0c4408e0600f601a166c 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  1762ee5fd21bb49f6593c2f0a0cc5810bfc4b8dd 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  1bdebb13b4cf981f0d69d93574a3ba317d9822ba 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  4652dc9e0a35a20780418d8e388595198a8f9ccd 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  83340342851598caf86f7c9e8e4d79f0e6027b20 
> 
> 
> Diff: https://reviews.apache.org/r/62112/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Sentry CLI

[Alexander Kolbasov]

I put current code for sentry CLI into akolb-cli branch. So far it supports
Hive privileges only. It would be great of some can add support for
Solr/Kafka privilege model as well. Any volunteers?

- Alex

Re: Review Request 61858: SENTRY-1898: Sentry no longer supports creating more than ~15 partitions at once

[Alexander Kolbasov]

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



We get all path changes via notifications. DOes this match the way 
notifications are defined in HMS?


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Line 302 (original), 302 (patched)
<https://reviews.apache.org/r/61858/#comment259721>

Is there any reason not to do this for permission changes?



sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
Line 16 (original), 16 (patched)
<https://reviews.apache.org/r/61858/#comment259722>

Do we need mediumtext? Would TEXT (64K max) be sufficient?



sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
Line 17 (original), 17 (patched)
<https://reviews.apache.org/r/61858/#comment259723>

Would VARCHAR2 work here?


- Alexander Kolbasov


On Aug. 23, 2017, 7:46 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61858/
> ---
> 
> (Updated Aug. 23, 2017, 7:46 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> change sql schema to allow path_change contain larger value
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  77ec491 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql
>  4afa2e0 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
>  8636fec 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
>  c3c374b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql
>  d168bf5 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 
> 69ef5b7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 
> 0db7ba9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> 183481a 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 
> cf4f0ed 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql
>  5974ed9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-2.0.0.sql
>  20c50b7 
> 
> 
> Diff: https://reviews.apache.org/r/61858/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 61862: SENTRY-1822 Allow multiple Sentry reporters.

[Alexander Kolbasov]

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

Review request for sentry, Brian Towles, Na Li, Sergio Pena, Vamsee Yarlagadda, 
and Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1822 Allow multiple Sentry reporters.


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java
 32a0664bda1156a089406c59fef8f07c807850f4 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61858: SENTRY-1898: Sentry no longer supports creating more than ~15 partitions at once

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Aug. 23, 2017, 7:46 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61858/
> ---
> 
> (Updated Aug. 23, 2017, 7:46 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> change sql schema to allow path_change contain larger value
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  77ec491 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql
>  4afa2e0 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
>  8636fec 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
>  c3c374b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql
>  d168bf5 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 
> 69ef5b7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 
> 0db7ba9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> 183481a 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 
> cf4f0ed 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql
>  5974ed9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-2.0.0.sql
>  20c50b7 
> 
> 
> Diff: https://reviews.apache.org/r/61858/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 61889: SENTRY-1895: Sentry should handle the case of multiple notifications with the same ID

[Alexander Kolbasov]

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



I think that it is more useful to get SHA1 on the original HMS event (JSON 
object). This will allow us to quickly commpare whether the new notification 
from HMS is the same as the one we already processed or not. Otherwise we need 
to convert it to PathsUpdate first and then do sha1 hash. WOuld it fit the code 
structure?


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
Line 84 (original), 84 (patched)
<https://reviews.apache.org/r/61889/#comment259845>

Is this a stray change?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
Line 69 (original), 73 (patched)
<https://reviews.apache.org/r/61889/#comment259870>

This field should be renamed - it is no longer an ID but hash.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
Lines 82 (patched)
<https://reviews.apache.org/r/61889/#comment259871>

s/will be/is/



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Line 299 (original), 299 (patched)
<https://reviews.apache.org/r/61889/#comment259869>

Please change the name to reflect the fact that this is not an ID but hash. 
Also, please add comment explaining what this is and why it is using CHAR(40)


- Alexander Kolbasov


On Aug. 24, 2017, 4:59 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61889/
> ---
> 
> (Updated Aug. 24, 2017, 4:59 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Na Li.
> 
> 
> Bugs: sentry-1895
> https://issues.apache.org/jira/browse/sentry-1895
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added two indexes to the NOTIFICATION_ID columns on the following tables:
> - SENTRY_HMS_NOTIFICATION_ID   (non-unique index)
> - SENTRY_PATH_CHANGE   (unique index)
> 
> Calculate the sha1 hex string on the MSentryPathChange and persist it.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  719c1acc18ce627f5228efb0d9cf47a7b5810a1b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  58878e55a23936bc0a2f15fa33883e655b255640 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  734ea7fd214cd577bf456cc84d27949ee332a468 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql
>  019b0406291f5143b92aa9b678c528da58db1602 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
>  a0223dc09046048582f395e8bd8e41cd73f462be 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
>  733cebfdf78d03c808a2c8ae04476669bd285416 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql
>  cd7afd4e3afb9aededd07506f82b43055abc2254 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 
> 44124c6d77c2e6c4185e4ef306ff4fbc20b5527b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 
> 3a7b46ee246d3347d371261a4bf727da4fb163fc 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> 7475a5de88abc267b39d7fafa48fd0b14c94d31b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 
> 20c4cd99b7933f612231afac938fba03632258d2 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql
>  794ff1778efb30468a5b7a23471fc71e99d42ab4 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-2.0.0.sql
>  065921e3e637ee78c1a118bf3e05a0af9a1d602e 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  8f60b5801b5f481bc3d225a64f411f1867aed141 
> 
> 
> Diff: https://reviews.apache.org/r/61889/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 61793: SENTRY-1894: Update field size in package.jdo for dataNucleus to match size in sql

[Alexander Kolbasov]

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



For MySQL I see this:

ALTER TABLE `SENTRY_DB_PRIVILEGE`
  ADD UNIQUE `SENTRY_DB_PRIV_PRIV_NAME_UNIQ` 
(`SERVER_NAME`,`DB_NAME`,`TABLE_NAME`,`COLUMN_NAME`,`URI`(250),`ACTION`,`WITH_GRANT_OPTION`);

Does it match your changes?

- Alexander Kolbasov


On Aug. 25, 2017, 7:25 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61793/
> ---
> 
> (Updated Aug. 25, 2017, 7:25 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> reduce field size `SERVER_NAME`,`DB_NAME`,`TABLE_NAME`,`COLUMN_NAME` in 
> `SENTRY_DB_PRIVILEGE` from 4000 to 128, and remove `URI` from being part of 
> the unique id
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  734ea7f 
> 
> 
> Diff: https://reviews.apache.org/r/61793/diff/2/
> 
> 
> Testing
> ---
> 
> run datanucleus schema tool. Without this change, it complains 
> "com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Specified key was 
> too long; max key length is 767 bytes. 
>  After those change, the datanucleus schema tool finishes successfully with 
> command "mvn datanucleus:schema-create -X -e"
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 61889: SENTRY-1895: Sentry should handle the case of multiple notifications with the same ID

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
Line 110 (original), 119 (patched)
<https://reviews.apache.org/r/61889/#comment259945>

Can we just use hash as the hashcode and skip the rest? It already 
incorporates the rest



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
Line 138 (original), 147 (patched)
<https://reviews.apache.org/r/61889/#comment259946>

Since hashCode incorporates the rest we can just return true here


- Alexander Kolbasov


On Aug. 25, 2017, 8:34 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61889/
> ---
> 
> (Updated Aug. 25, 2017, 8:34 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Na Li.
> 
> 
> Bugs: sentry-1895
> https://issues.apache.org/jira/browse/sentry-1895
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Added two indexes to the NOTIFICATION_ID columns on the following tables:
> - SENTRY_HMS_NOTIFICATION_ID   (non-unique index)
> - SENTRY_PATH_CHANGE   (unique index)
> 
> Calculate the sha1 hex string on the MSentryPathChange and persist it.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UniquePathsUpdate.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java
>  7ea75a093f49f00cefec67ae3884dfc82650f700 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  f2368b7d2be8b8926ba31562bf7d3493f7d36378 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  58878e55a23936bc0a2f15fa33883e655b255640 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  734ea7fd214cd577bf456cc84d27949ee332a468 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
>  4fe62bf7ac4a1bde84836d7e65ac6069751aceac 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  d7acaea7c32c34131c3f9d5ed85b0b12244b0f1d 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  9e28da4d96c873afad7279e6f75cd6a31627b7b1 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql
>  019b0406291f5143b92aa9b678c528da58db1602 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
>  a0223dc09046048582f395e8bd8e41cd73f462be 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
>  733cebfdf78d03c808a2c8ae04476669bd285416 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql
>  cd7afd4e3afb9aededd07506f82b43055abc2254 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 
> 44124c6d77c2e6c4185e4ef306ff4fbc20b5527b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 
> 3a7b46ee246d3347d371261a4bf727da4fb163fc 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> 7475a5de88abc267b39d7fafa48fd0b14c94d31b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 
> 20c4cd99b7933f612231afac938fba03632258d2 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql
>  794ff1778efb30468a5b7a23471fc71e99d42ab4 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-2.0.0.sql
>  065921e3e637ee78c1a118bf3e05a0af9a1d602e 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
>  6ff79f86f9aa3c8e0e840686a03d610a9fde6f01 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  8f60b5801b5f481bc3d225a64f411f1867aed141 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  c5fedfc694

Re: Review Request 61827: SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Aug. 23, 2017, 7:29 p.m., Misha Dmitriev wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61827/
> ---
> 
> (Updated Aug. 23, 2017, 7:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1892
> https://issues.apache.org/jira/browse/SENTRY-1892
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java
>  2a89368208bcef5b537cb0c2d59fd14b1735f435 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  6e71561e36d7f235afb14961299bfc23c03607a6 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  bf6c3dec35c7d22b9bd0e3f863925f7ff3e94515 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift 
> 22dea02ea003530513d7fb270eb0ca0ce38957e9 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  a0d7bdcd256a17558178ebc5238b1f3da9fa5e9b 
> 
> 
> Diff: https://reviews.apache.org/r/61827/diff/2/
> 
> 
> Testing
> ---
> 
> Ran 'mvn install' locally, all tests passed.
> 
> 
> Thanks,
> 
> Misha Dmitriev
> 
>

Re: Review Request 61827: SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry

[Alexander Kolbasov]

> On Aug. 23, 2017, 6:35 p.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Lines 537 (patched)
> > <https://reviews.apache.org/r/61827/diff/1/?file=1801885#file1801885line541>
> >
> > Looks like most callers just care whether size() != 0, so you may have 
> > even simpler isEmpty() function.
> 
> Misha Dmitriev wrote:
> In principle, yes, but let's have some more flexibility for the future. 
> Looks like size == 1 is a very frequent case and maybe the rest of the Sentry 
> code will benefit from easily accessible knowledge about it.

Can we have two functions - size() and isEmpty()?


- Alexander


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


On Aug. 22, 2017, 11:02 p.m., Misha Dmitriev wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61827/
> ---
> 
> (Updated Aug. 22, 2017, 11:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1892
> https://issues.apache.org/jira/browse/SENTRY-1892
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java
>  2a89368208bcef5b537cb0c2d59fd14b1735f435 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  6e71561e36d7f235afb14961299bfc23c03607a6 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  bf6c3dec35c7d22b9bd0e3f863925f7ff3e94515 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift 
> 22dea02ea003530513d7fb270eb0ca0ce38957e9 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  a0d7bdcd256a17558178ebc5238b1f3da9fa5e9b 
> 
> 
> Diff: https://reviews.apache.org/r/61827/diff/1/
> 
> 
> Testing
> ---
> 
> Ran 'mvn install' locally, all tests passed.
> 
> 
> Thanks,
> 
> Misha Dmitriev
> 
>

Re: Review Request 61827: SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry

[Alexander Kolbasov]

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



This is the last comment I have - looks good!


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 537 (patched)
<https://reviews.apache.org/r/61827/#comment259698>

Looks like most callers just care whether size() != 0, so you may have even 
simpler isEmpty() function.


- Alexander Kolbasov


On Aug. 22, 2017, 11:02 p.m., Misha Dmitriev wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61827/
> ---
> 
> (Updated Aug. 22, 2017, 11:02 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1892
> https://issues.apache.org/jira/browse/SENTRY-1892
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java
>  2a89368208bcef5b537cb0c2d59fd14b1735f435 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  6e71561e36d7f235afb14961299bfc23c03607a6 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java
>  bf6c3dec35c7d22b9bd0e3f863925f7ff3e94515 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift 
> 22dea02ea003530513d7fb270eb0ca0ce38957e9 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  a0d7bdcd256a17558178ebc5238b1f3da9fa5e9b 
> 
> 
> Diff: https://reviews.apache.org/r/61827/diff/1/
> 
> 
> Testing
> ---
> 
> Ran 'mvn install' locally, all tests passed.
> 
> 
> Thanks,
> 
> Misha Dmitriev
> 
>

Re: Review Request 62044: SENTRY-1913: Incorrect constraints on AUTHZ_PATHS_MAPPING.AUTHZ_OBJ_NAME

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 1, 2017, 10:26 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62044/
> ---
> 
> (Updated Sept. 1, 2017, 10:26 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1913: Incorrect constraints on AUTHZ_PATHS_MAPPING.AUTHZ_OBJ_NAME
> 
> * Adds NOT NULL constraint on AUTHZ_OBJ_NAME
> * Creates a multiple unique index on fields AUTHZ_OBJ_NAME, AUTHZ_SNAPSHOT_ID
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  59f68263deb1df050daa82eb0c2374943a55d743 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql
>  d738230a2723cca8b89ca4382e59d59ba063b1d9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  84eac33b40f4a565010c5e9686bc3869028f2b96 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql
>  d7d9679cf29e79610d96bc75b0607b693a1a7c26 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql
>  cf961f3b7458c159262eaf63dfc148923a5562e5 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 
> 93d6e7039e5b00e39933cd7ad46228192ae46ef7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 
> b4e39fd123a3da3b5775c08c90cd61473e6accbb 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql 
> 4326a3877a93511d49201aec1bd6023951ca5ab8 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 
> 92676a5d130d46d5422d969cad26f9a2aeafbd76 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql
>  05eb7513375a110852a5038c2244fd75a8521f5f 
> 
> 
> Diff: https://reviews.apache.org/r/62044/diff/1/
> 
> 
> Testing
> ---
> 
> Pre-commit tests in progress.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Re: [DISCUSSION] Move to JDK8 and Datanucleus 4

[Alexander Kolbasov]

+1 for both.

I would be still against actually allowing Java 8 specific code since
people may want to backport fixes to old releases which might still use
Java 7. Allowing Java-8 exclusive features may make this process
complicated. Lambdas are cute, but we can do without them for a while. It
would be good to have some automatic enforcement so that Java8 features do
not leak in accidentally.

On Tue, Aug 29, 2017 at 3:56 PM, Na Li  wrote:

> +1 for moving to datanucleus 4 and Java 8
>
> On Tue, Aug 29, 2017 at 3:48 PM, Sergio Pena 
> wrote:
>
> > Hi All,
> >
> > There is a JIRA request (SENTRY-1893
> > ) looking to add
> > support
> > for JDK8 and make it as the default and minimum JDK version to use. JDK7
> > has reached the end of life, and many other Apache components (including
> > Hive 2.1) have already switched to JDK8 as the minimum version.
> >
> > I would like to use this email thread to discuss if we should follow this
> > trend and switch to JDK8 on our current Sentry 2.0 development. Moving to
> > JDK8 will also allow us to use the new API that brings, such as lambda
> > functions.
> >
> > Also, in order to support JDK8, we should switch to Datanucleus 4 because
> > Datanucleus 3 have some problems with it. Datanucleus 3 is also old, and
> > other components already switched to version 4 as well. We already have
> > support for version 4, so the question here is if we should drop
> > Datanucleus 3 support and just use version 4 as the default.
> >
> > Sentry 2.0 is our current major version development, so it makes sense to
> > do this move in this version.
> >
> > What do you all think?
> >
> > - Sergio
> >
>

Draft of the quarterly ASF report for Apache Sentry

[Alexander Kolbasov]

Here is the draft of the quarterly Apache Sentry ASF report. Please let me
know if you have any comments or issues with it.

Thanks,

- Alex.

## Description:

Apache Sentry is a highly modular system for providing fine grained role
based authorization to both data and metadata stored on an Apache Hadoop
Cluster.

## Issues:

There are no issues requiring board attention at this time

## Activity:

 - Version 1.8 released
 - Preparation for releasing version 2.0 based on Hive 2 and including HA
support

## Health report:

   Development activity seems pretty consistent;
   No new developers since last time

## PMC changes:

 - Currently 35 PMC members.
 - Vadim Spector was added to the PMC on Sun Jun 11 2017

## Committer base changes:

 - Currently 38 committers.
 - New commmitters:
- Kalyan Kalvagadda was added as a committer on Wed Jun 21 2017
- Sergio Peña was added as a committer on Mon Aug 28 2017
- Vadim Spector was added as a committer on Fri Jun 02 2017

## Releases:

 - 1.8.0 was released on Sun Aug 06 2017
 - 1.7.0 was released on Tue Jun 14 2016

## Mailing list activity:

Most of the activity was about 1.8/2.0 releases and about finishing up HA
work.

## JIRA activity:

 - 119 JIRA tickets created in the last 3 months
 - 125 JIRA tickets closed/resolved in the last 3 months

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving roles for groups

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 1811 (original), 1811 (patched)
<https://reviews.apache.org/r/61863/#comment260460>

Can we use

pm.setDetachAllOnCommit(false);

here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 1822 (original), 1822 (patched)
<https://reviews.apache.org/r/61863/#comment260459>

What does this function do? Please add documentation. The code is a bit 
non-trivial, please explain what you are doing and why.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 1823 (original), 1823 (patched)
<https://reviews.apache.org/r/61863/#comment260461>

Do we need results of the query after close?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1825 (patched)
<https://reviews.apache.org/r/61863/#comment260462>

What is the goal of this? Previously in this case we returned an empty set.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1828 (patched)
<https://reviews.apache.org/r/61863/#comment260463>

`sentrGroup` is a weird name. Of course this is sentry, so group is a 
perfect name.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1833 (patched)
<https://reviews.apache.org/r/61863/#comment260464>

Do we need to use FetchGroups here? Why do you think it is beneficial?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 1839 (patched)
<https://reviews.apache.org/r/61863/#comment260465>

Just use HashSet<>(result)



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 1808 (original), 1808 (patched)
<https://reviews.apache.org/r/61863/#comment260466>

You should also have a test with null groups and with empty groups.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 1811 (patched)
<https://reviews.apache.org/r/61863/#comment260467>

how about number of groups? You are testing with a single group.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 1818 (patched)
<https://reviews.apache.org/r/61863/#comment260468>

Here and above, use new HashSet<>() instead of Sets.newHashSet()



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 1842 (patched)
<https://reviews.apache.org/r/61863/#comment260469>

Do you need to remove all these roles at the end of the test? Other tests 
may fail because some role exists.


- Alexander Kolbasov


On Aug. 31, 2017, 9:55 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> -------
> 
> (Updated Aug. 31, 2017, 9:55 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Vamsee Yarlagadda, and Vadim 
> Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. However, DataNucleus takes too long and 
> the fetch doesn't seem to be lazy. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  d7acaea7c 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  8f60b5801 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61973: SENTRY-1888: Sentry might not fetch all HMS duplicated events IDs when requested

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Aug. 31, 2017, 6:13 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61973/
> ---
> 
> (Updated Aug. 31, 2017, 6:13 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Na Li.
> 
> 
> Bugs: sentry-1888
> https://issues.apache.org/jira/browse/sentry-1888
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Approach:
> The proposed solution will be to request notifications from the last ID seen 
> again. This way we could bring current duplicates and apply them on Sentry. 
> We have the risk to miss duplicates that were committed much time later, but 
> we cannot trust on those duplicates as they will not know the order of the 
> time they were committed.
> 
> The patch adds a new helper class called HiveNotificationFetcher that allow 
> us to bring new notifications and apply a filter to those notifications that 
> were already processed by Sentry.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UniquePathsUpdate.java
>  7dae2f538602f4c264084791fb45bb6891a34941 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  593b92f96b47f959266280bce3d0809f6a80c362 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  35da6fc7908ec7498d1fd658d75b62028df35751 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4a8fb953ce486e1aeb1042884de56872b5539cd0 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  f56384a99e128b3e9880cbc5692107d61f2f500f 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java
>  77a2bbb214e23ca146c2934ee4d3bf15e592906f 
> 
> 
> Diff: https://reviews.apache.org/r/61973/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 62007: SENTRY-1909 Improvements for memory usage when full path snapshot is sent from Sentry to NN

[Alexander Kolbasov]

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

(Updated Aug. 31, 2017, 6:54 a.m.)


Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
---

Fixed SentryStore tests


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


Repository: sentry


Description
---

SENTRY-1909 Improvements for memory usage when full path snapshot is sent from 
Sentry to NN


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 898c7bec2e35e6f1424478666282ba78222da709 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 20b3e108cc976207a3809bc6a214a34e10788200 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 e403d7cca5dca5c39785490f92e562dc9a3b4daf 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
 fee5279a7cd3d60df4e60a5f25d2e8604c37d04b 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java
 409a557fe89d85df888c1f16b3f5aacdd9aa96b0 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 593b92f96b47f959266280bce3d0809f6a80c362 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 760a2b55e98ac1fb305666cbbedfba11d26f2fcc 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 2cd18ea9d2ca40b5f46aaafde532fb3bf9769c07 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4a8fb953ce486e1aeb1042884de56872b5539cd0 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 3da6a4e998d36cada7e9ea77285b11f07cea5f25 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
 9d7bddd339513180e627286d8a749b7814c824f2 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 7deccb064fd75b39af779796d9e95caf9c718b32 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
 f56384a99e128b3e9880cbc5692107d61f2f500f 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 62007: SENTRY-1909 Improvements for memory usage when full path snapshot is sent from Sentry to NN

[Alexander Kolbasov]

> On Aug. 31, 2017, 6:28 a.m., Misha Dmitriev wrote:
> > In general I wonder whether instead of the old Set<> we should now use 
> > Collection<> rather than the more explicit List<>. I think using a 
> > supertype like Collection<> makes sense only if it's used for a method 
> > parameter that can receive either List or Set. Do we really have such 
> > methods? Even if there are some, I think in most other places you can 
> > replace Collection with List safely.
> > 
> > I think the best practice suggests that it's better to be more specific 
> > about a variable type T unless this variable either really can receive 
> > different subtypes of T, or we think it may, in principle (e.g. it's better 
> > to use a Map rather than HashMap in some library method, because someone 
> > may want to pass it a LinkedHashMap, ConcurrentHashMap, etc.) But if here 
> > in certain places we very explicitly want to use ArrayLists rather than 
> > HashSets for performance reasons, I think it's better to not put the reader 
> > in any doubt and use type List rather than Collection.

This case is interesting because we sometimes use ArrayList and sometimes Set 
as the underlying collection, that's why I wanted to use Collections as the 
type here.


> On Aug. 31, 2017, 6:28 a.m., Misha Dmitriev wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
> > Line 142 (original), 143 (patched)
> > <https://reviews.apache.org/r/62007/diff/1/?file=1808325#file1808325line143>
> >
> > I don't like these type casts at all. Type casts are ok in certain 
> > cases when we really don't know in advance the actual type of the given 
> > object. But here, at a minimum, we can change the type of the 'image' 
> > parameter to 'Map<String, Set>', no? Same in many other methods 
> > below.

This doesn't work since Map<String, Set> is not compatible with 
Map<String, Collection>. Here we know exactly that we are using sets 
but interfaces require collections. I don't know how to work around this.


- Alexander


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


On Aug. 31, 2017, 1:38 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62007/
> ---
> 
> (Updated Aug. 31, 2017, 1:38 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, Misha Dmitriev, Na Li, 
> Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1909
> https://issues.apache.org/jira/browse/SENTRY-1909
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1909 Improvements for memory usage when full path snapshot is sent 
> from Sentry to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  898c7bec2e35e6f1424478666282ba78222da709 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
>  20b3e108cc976207a3809bc6a214a34e10788200 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
>  e403d7cca5dca5c39785490f92e562dc9a3b4daf 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  fee5279a7cd3d60df4e60a5f25d2e8604c37d04b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java
>  409a557fe89d85df888c1f16b3f5aacdd9aa96b0 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  593b92f96b47f959266280bce3d0809f6a80c362 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  760a2b55e98ac1fb305666cbbedfba11d26f2fcc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  2cd18ea9d2ca40b5f46aaafde532fb3bf9769c07 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4a8fb953ce486e1aeb1042884de56872b5539cd0 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  3da6a4e998d36cada7e9ea77285b11f07cea

[DISCUSS] Who is using Sentry outside of Cloudera distribution?

[Alexander Kolbasov]

I'd like to get a feel on when and how Apache Sentry is used outside of
Cloudera distribution, so if anyone is using Apache Sentry not within the
CDH ecosystem, I would appreciate if you share your experience.

Thanks,

- ALex

Re: Proposal to have sentry 1.9 release.

[Alexander Kolbasov]

The major reason why we wanted to move to Sentry 2.0 was the dependency on
Hive version. Sentry HA completely relies on HMS notifications which are
not available in Hive 1, more over it relies on fixes that are only
available in Hive 2.4. Sentry 1 should work with Hive 1, so we can't make
Sentry HA work with Hive 1.

- Sasha

On Thu, Oct 5, 2017 at 7:15 AM, Kalyan Kumar Kalvagadda <
kkal...@cloudera.com> wrote:

> Colm,
>
> If we have consensus on releasing sentry 1.9.0, I can work on release
> process which takes couple of weeks from now.
>
> -Kalyan
>
> On Thu, Oct 5, 2017 at 3:50 AM, Colm O hEigeartaigh 
> wrote:
>
> > Fine with me, it makes sense to get the HA work out there if it is ready
> to
> > be used. What kind of time-line are you thinking of for 1.9.0?
> >
> > Colm.
> >
> > On Thu, Oct 5, 2017 at 4:37 AM, Sergio Pena 
> > wrote:
> >
> > > It sounds good to do. One of the reasons to do sentry 2.0 was the
> > > integration with hive-authz2, but this is taking time for quality
> > concerns.
> > > Perhaps is good to cut the branch for 1.9 now before starting to add
> > other
> > > incompatible changes to the master branch.
> > >
> > > +1 to release 1.9 + ha work.
> > >
> > > On Wed, Oct 4, 2017 at 5:19 PM, Kalyan Kumar Kalvagadda <
> > > kkal...@cloudera.com> wrote:
> > >
> > > > Hi all,
> > > >
> > > > I understand that the major areas that still needs significant amount
> > > work
> > > > on sentry 2.0 are
> > > >
> > > >1. Integrating with Hive 2.0
> > > >2. Updating the java version
> > > >3. Integrating with Solr-7
> > > >
> > > >
> > > > *Integrating with Hive 2.0: *While testing stuff with Hive locally,
> we
> > > > found couple of issues with Hive-2 and sentry 2.0 integration using
> > > auth-v2
> > > > interface. There were couple of fixes that are needed from Hive side,
> > > some
> > > > of them are already fixed in Hive 3.0 and some are yet to be fixed.
> > > Bottom
> > > > line is that we need some help from Hive community in fixing some
> > issues
> > > > and back-porting them to some Hive-2.x release before we actually try
> > to
> > > > continue testing it with new Hive-2.x release. This could take quite
> > some
> > > > time
> > > >
> > > > *Integrating with Solr-7: *We happened to talk to contributors of
> this
> > > > feature. They might some time to actually get it ready.
> > > >
> > > > These outstanding work need not delay the release of sentry-HA which
> is
> > > > well tested locally at cloudera. This feature is something that the
> > > > community is waiting for quite some time. Why don't we release
> current
> > > > master as sentry 1.9 release?
> > > >
> > > > Sentry 1.9.0 if released, would below features.
> > > >
> > > >1. Sentry-HA
> > > >2. Integration with Hive 2.0 using auth-V1.
> > > >
> > > > Seeking inputs from every one in this regard.
> > > >
> > > >
> > > > -Kalyan
> > > >
> > >
> >
> >
> >
> > --
> > Colm O hEigeartaigh
> >
> > Talend Community Coder
> > http://coders.talend.com
> >
>

Re: Do Sentry sync-up meetings exist?

[Alexander Kolbasov]

It is https://apache-sentry.slack.com <https://apache-sentry.slack.com/> - I 
created room in my private slack account - sent Colm an invite.


> On Oct 4, 2017, at 1:36 AM, Colm O hEigeartaigh <cohei...@apache.org> wrote:
> 
> Where is the Slack room? I don't see it in the ASF team.
> 
> Colm.
> 
> On Wed, Oct 4, 2017 at 1:15 AM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
> 
>> Sravya used to have meetings over hangout, it was attended by Colin Ma and
>> some other folks. It is definitely a good idea if there is an interest.
>> 
>> Also a reminder that we have a slack room for discussions.
>> 
>> On Tue, Oct 3, 2017 at 2:50 PM, Sergio Pena <sergio.p...@cloudera.com>
>> wrote:
>> 
>>> Hi All,
>>> 
>>> Did we have any kind of sync-up meetings among the Sentry community in
>> the
>>> past?
>>> 
>>> I've been in other Apache communities where they have these google chat
>>> sync-up meetings every 1 or 2 months to talk about what they'd like to do
>>> next in their products. Do you guys think this should be a reasonable
>> thing
>>> to do?
>>> 
>>> - Sergio
>>> 
>> 
> 
> 
> 
> -- 
> Colm O hEigeartaigh
> 
> Talend Community Coder
> http://coders.talend.com

Re: Do Sentry sync-up meetings exist?

[Alexander Kolbasov]

I don’t want to make it too official yet - this is experimental thing using my 
private slack account - and so far it isn’t very active. Let’s test-drive it 
for a while and then see where we can put it.

- Alex

> On Oct 4, 2017, at 3:21 PM, Sergio Pena <sergio.p...@cloudera.com> wrote:
> 
> Sasha, should we add this information about Slack rooms here and how to
> request an invitation?
> http://sentry.apache.org/community/get_involved.html
> 
> On Wed, Oct 4, 2017 at 5:19 PM, Alexander Kolbasov <ak...@cloudera.com>
> wrote:
> 
>> It is https://apache-sentry.slack.com <https://apache-sentry.slack.com/>
>> - I created room in my private slack account - sent Colm an invite.
>> 
>> 
>>> On Oct 4, 2017, at 1:36 AM, Colm O hEigeartaigh <cohei...@apache.org>
>> wrote:
>>> 
>>> Where is the Slack room? I don't see it in the ASF team.
>>> 
>>> Colm.
>>> 
>>> On Wed, Oct 4, 2017 at 1:15 AM, Alexander Kolbasov <ak...@cloudera.com>
>>> wrote:
>>> 
>>>> Sravya used to have meetings over hangout, it was attended by Colin Ma
>> and
>>>> some other folks. It is definitely a good idea if there is an interest.
>>>> 
>>>> Also a reminder that we have a slack room for discussions.
>>>> 
>>>> On Tue, Oct 3, 2017 at 2:50 PM, Sergio Pena <sergio.p...@cloudera.com>
>>>> wrote:
>>>> 
>>>>> Hi All,
>>>>> 
>>>>> Did we have any kind of sync-up meetings among the Sentry community in
>>>> the
>>>>> past?
>>>>> 
>>>>> I've been in other Apache communities where they have these google chat
>>>>> sync-up meetings every 1 or 2 months to talk about what they'd like to
>> do
>>>>> next in their products. Do you guys think this should be a reasonable
>>>> thing
>>>>> to do?
>>>>> 
>>>>> - Sergio
>>>>> 
>>>> 
>>> 
>>> 
>>> 
>>> --
>>> Colm O hEigeartaigh
>>> 
>>> Talend Community Coder
>>> http://coders.talend.com
>> 
>> 

Re: Review Request 62745: SENTRY-1210 - Refactor the SentryShellSolr and SentryShellKafka

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java
Lines 110 (patched)
<https://reviews.apache.org/r/62745/#comment263988>

I think this should be more explicit and fail if the type isn't kafka or 
solr



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java
Lines 117 (patched)
<https://reviews.apache.org/r/62745/#comment263989>

    Ditto.


- Alexander Kolbasov


On Oct. 3, 2017, 10:41 a.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62745/
> ---
> 
> (Updated Oct. 3, 2017, 10:41 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1210
> https://issues.apache.org/jira/browse/SENTRY-1210
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> For generic model, one shell like SentryShellGeneric should be covered all 
> related components, eg, solr, kafka. SentryShellSolr and SentryShellKafka 
> should be replaced with the SentryShellGeneric
> 
> 
> Diffs
> -
> 
>   bin/sentryShell f0661568 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java
>  f6e5d1b6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java
>  5385f7d7 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java
>  6ddc1def 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellKafka.java
>  7db54260 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java
>  d4e26e82 
> 
> 
> Diff: https://reviews.apache.org/r/62745/diff/1/
> 
> 
> Testing
> ---
> 
> Tested that the binary "sentryShell" can list privileges correctly for both 
> Kafka and Solr.
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Re: Do Sentry sync-up meetings exist?

[Alexander Kolbasov]

Sravya used to have meetings over hangout, it was attended by Colin Ma and
some other folks. It is definitely a good idea if there is an interest.

Also a reminder that we have a slack room for discussions.

On Tue, Oct 3, 2017 at 2:50 PM, Sergio Pena 
wrote:

> Hi All,
>
> Did we have any kind of sync-up meetings among the Sentry community in the
> past?
>
> I've been in other Apache communities where they have these google chat
> sync-up meetings every 1 or 2 months to talk about what they'd like to do
> next in their products. Do you guys think this should be a reasonable thing
> to do?
>
> - Sergio
>

Re: Code consolidation query

[Alexander Kolbasov]

Interesting - both were introduced by Colin Ma. I definitely agree that we
do not need two duplicates of the same similar functionality. The exception
part is interesting - the validator throws unchecked exceptions.

The version in SentryServiceUtil is used only in import policy code path
where validation should be always ok.

The second one is used by shell only.

There is another internal version in SentryStore:

private void convertToTSentryPrivilege(MSentryPrivilege mSentryPrivilege,
TSentryPrivilege privilege) {
  privilege.setCreateTime(mSentryPrivilege.getCreateTime());
  privilege.setAction(fromNULLCol(mSentryPrivilege.getAction()));
  privilege.setPrivilegeScope(mSentryPrivilege.getPrivilegeScope());
  privilege.setServerName(fromNULLCol(mSentryPrivilege.getServerName()));
  privilege.setDbName(fromNULLCol(mSentryPrivilege.getDbName()));
  privilege.setTableName(fromNULLCol(mSentryPrivilege.getTableName()));
  privilege.setColumnName(fromNULLCol(mSentryPrivilege.getColumnName()));
  privilege.setURI(fromNULLCol(mSentryPrivilege.getURI()));
  if (mSentryPrivilege.getGrantOption() != null) {

privilege.setGrantOption(TSentryGrantOption.valueOf(mSentryPrivilege.getGrantOption().toString().toUpperCase()));
  } else {
privilege.setGrantOption(TSentryGrantOption.UNSET);
  }
}



On Mon, Oct 2, 2017 at 10:07 AM, Colm O hEigeartaigh 
wrote:

> Hi all,
>
> I'm preparing a patch to consolidate some duplicate code in
> sentry-provider-db, and have a question:
>
> CommandUtil.convertToTSentryPrivilege:
>
> https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java#L38
>
> is identical to SentryServiceUtil.convertToTSentryPrivilege:
>
> https://github.com/apache/sentry/blob/186e75543a23f286e24c56f7385909
> 9405b7b73a/sentry-provider/sentry-provider-db/src/main/
> java/org/apache/sentry/service/thrift/SentryServiceUtil.java#L56
>
> apart from two differences:
>
> a) The CommandUtil version sets:
> tSentryPrivilege.setAction(AccessConstants.ALL); for the
> PRIVILEGE_URI_NAME case.
> b) The CommandUtil version "validates" the privilege hierarchy, throwing an
> exception if one of the names is not specified.
>
> Ideally I'd like to remove CommandUtil altogether and just reference the
> methods in SentryServiceUtil. Should these extra pieces also be in
> SentryServiceUtil? Or is there a reason that they are different?
>
> Colm.
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: questions on sentry

[Alexander Kolbasov]

Hi Chen,

Apache Sentry goal is to provide security features for other Apache
products - mainly Apache {Hive, Impala, Solr, Kafka}. It doesn't provide
HDFS access protections.

- Alex.

On Wed, Oct 18, 2017 at 12:05 PM, Chen Song  wrote:

> I have a few questions on Sentry. I have some thoughts in my mind but just
> want to confirm with the community.
>
> 1. Does Sentry have a way to block HDFS superuser from access HDFS
> directories?
> 2. Does Sentry support blacklisting a list of IPs from Hive or HDFS access?
>
> After a bit search, I don't find a way for both but I want to double check
> again.
>
> Chen
>

Re: questions on sentry

[Alexander Kolbasov]

Hi Chen,

Apache Sentry goal is to provide security features for other Apache
products - mainly Apache {Hive, Impala, Solr, Kafka}. It doesn't provide
HDFS access protections.

- Alex.

On Wed, Oct 18, 2017 at 12:05 PM, Chen Song  wrote:

> I have a few questions on Sentry. I have some thoughts in my mind but just
> want to confirm with the community.
>
> 1. Does Sentry have a way to block HDFS superuser from access HDFS
> directories?
> 2. Does Sentry support blacklisting a list of IPs from Hive or HDFS access?
>
> After a bit search, I don't find a way for both but I want to double check
> again.
>
> Chen
>