Re: [DISCUSS] Merging Sentry HA branch with master

[Alexander Kolbasov]

I would like to make a more concrete proposal and I am interested in opinion 
from Sentry PMC members on this.

I would propose the following approach for merging Sentry HA into Sentry master:

Cherry-pick any commits that happened since sentry-ha-redesign was forked, 
except a few described below
Exclude big refactoring commit (SENTRY-1205) and related commits (SENTRY-1436, 
SENTRY-1438, SENTRY-1406)
Rename master to a dev branch
Rename sentry-ha-redesign to master

What does community think about such approach?

- Alex


> On Mar 22, 2017, at 1:43 PM, Alexander Kolbasov  wrote:
> 
> Hello,
> 
> I would like to start the discussion on merging sentry-ha-redesign branch 
> with master.
> 
> As of now most of the changes from master are merged into sentry-ha-redesign. 
> The major missing part is SENTRY-1205 (Refactor the code for 
> sentry-provider-db and create sentry-service module) and associated issues. 
> This refactoring is very hard to port, especially since there is very little 
> information in the JIRA on why it was done and how it was done - was it 
> merely moving files around or more then that. I would seriously consider not 
> including this change in 1.8.
> 
> So in regards to the merge we have several options:
> 
> Attempt to merge master into sentry-ha-redesign, resolve any conflicts and 
> later commit the merge to master. This will cause merge commit on master
> Finish work on sentry-ha-redesign, make sure that relevant commits are ported 
> from master, and then making this a master branch and making current master a 
> special branch left for reference purposes. This will likely leave 
> SENTRY-1205 and related issues out.
> What does community think about this?
> 
> - Alex

Re: Review Request 58164: SENTRY-1638 Update SQL script of MSentryPathChange table to add a column for notification ID

[Alexander Kolbasov]

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



I don't see any changes to package.jdo announced at the header - are they 
missing?


sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql
Line 204 (original), 204 (patched)
<https://reviews.apache.org/r/58164/#comment244055>

Why did you remove all the quotes?


- Alexander Kolbasov


On April 4, 2017, 5:05 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58164/
> ---
> 
> (Updated April 4, 2017, 5:05 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and Na Li.
> 
> 
> Bugs: SENTRY-1638
> https://issues.apache.org/jira/browse/SENTRY-1638
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> UpdateD SQL script of MSentryPathChange table to add a column for 
> notification ID. Wich include changes to jdo defination and also 
> MSentryPermChange class.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.derby.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.mysql.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.oracle.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.postgres.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 547bbe8136186658e7fe76ab24934157ea5300ff 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  5376c166659b3e8e373c8c20818f5b1290af90c9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-1.7.0-to-1.8.0.sql
>  dc3021a57ac9ea8145379c437ae6a94131c9ae5e 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-1.7.0-to-1.8.0.sql
>  ef1c651da8998eb57b5d0ca3463317d07c1b3a24 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-1.7.0-to-1.8.0.sql
>  f4a50de14dd0f961befaded394c6a01c2f459317 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-1.7.0-to-1.8.0.sql
>  db010bcd7792c9370478a2061e22d14bb78e3184 
> 
> 
> Diff: https://reviews.apache.org/r/58164/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58164: SENTRY-1638 Update SQL script of MSentryPathChange table to add a column for notification ID

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 6, 2017, 12:05 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58164/
> ---
> 
> (Updated April 6, 2017, 12:05 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and Na Li.
> 
> 
> Bugs: SENTRY-1638
> https://issues.apache.org/jira/browse/SENTRY-1638
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> UpdateD SQL script of MSentryPathChange table to add a column for 
> notification ID.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.derby.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.mysql.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.oracle.sql
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1638.postgres.sql
>  PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 547bbe8136186658e7fe76ab24934157ea5300ff 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  5376c166659b3e8e373c8c20818f5b1290af90c9 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-1.7.0-to-1.8.0.sql
>  dc3021a57ac9ea8145379c437ae6a94131c9ae5e 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-1.7.0-to-1.8.0.sql
>  ef1c651da8998eb57b5d0ca3463317d07c1b3a24 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-1.7.0-to-1.8.0.sql
>  f4a50de14dd0f961befaded394c6a01c2f459317 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-1.7.0-to-1.8.0.sql
>  db010bcd7792c9370478a2061e22d14bb78e3184 
> 
> 
> Diff: https://reviews.apache.org/r/58164/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58130: SENTRY-1684 FullUpdateInitializer has a race condition in handling results list

[Alexander Kolbasov]

> On April 6, 2017, 2:03 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 311 (original), 314 (patched)
> > <https://reviews.apache.org/r/58130/diff/1/?file=1682896#file1682896line321>
> >
> > Under what condition will the taskCounter be negative? Should we throw 
> > exception if it is negative?

It would never be negative. It starts non-negative and will go to zero. The 
change expresses the desied condition more clearly - we are waiting until the 
value becomes zero.


- Alexander


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


On April 1, 2017, 12:35 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58130/
> ---
> 
> (Updated April 1, 2017, 12:35 a.m.)
> 
> 
> Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, and Vadim 
> Spector.
> 
> 
> Bugs: SENTRY-1684
> https://issues.apache.org/jira/browse/SENTRY-1684
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1684 FullUpdateInitializer has a race condition in handling results 
> list
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  146cea2b9467ce82b69bbf402933b1aa350bcd46 
> 
> 
> Diff: https://reviews.apache.org/r/58130/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58093: SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw RuntimeExceptio

[Alexander Kolbasov]

> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 137 (original), 135 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681748#file1681748line141>
> >
> > do you want to print out the value of "retries +1"? Does "retries + 1" 
> > gives you want you want?

It does - otherwise I'll see that it failed after 0 retries


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 322 (original), 319 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681748#file1681748line327>
> >
> > do you need to update testing cases in TestFullUpdateInitializer that 
> > verifies the exception type?

Hmm, the tests are passing fine. I guess tests do not check for this.


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 303 (original), 303 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681749#file1681749line303>
> >
> > since you changed the possible exception, you need to update the 
> > commnet at line 303

Good point, this should be updated.


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 310 (original), 310 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681749#file1681749line310>
> >
> > It would be easier to debug if you log the beginning of getting full 
> > snapshot.

Sure, we can add the log.


> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 312 (original), 312 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681749#file1681749line312>
> >
> > My impression is that it takes some time to get the full snapshot. 
> > Should we measure the time it takes to get the full snapshot? And show the 
> > duration in the log message?

I think we should provide this as a metric. Logs are time stamped - this could 
be sufficient, but having this as a metric is useful - then we can get it 
afterwords. I'll file a JIRA on this.


- Alexander


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


On March 31, 2017, 5:07 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58093/
> ---
> 
> (Updated March 31, 2017, 5:07 a.m.)
> 
> 
> Review request for sentry, Lei Xu, Hao Hao, kalyan kumar kalvagadda, and Na 
> Li.
> 
> 
> Bugs: SENTRY-1676
> https://issues.apache.org/jira/browse/SENTRY-1676
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw 
> RuntimeExceptio
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  146cea2b9467ce82b69bbf402933b1aa350bcd46 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
> 
> 
> Diff: https://reviews.apache.org/r/58093/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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



I put some high-level comments in the SENTRY-1593

You can take a look at the *prototype* of suggested changes in 
SENTRY-1593-akolb branch. This compiles, but is just a prototype - it needs 
work.

- Alexander Kolbasov


On April 6, 2017, 1:59 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 6, 2017, 1:59 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 9d18063 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  6cea596 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  636de40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  12175f7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  038bca7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClientTransportDefaultImpl.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyStoreConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java
>  ab12bf4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  28b1224 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
>  2a18b15 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java
>  4dc99a2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java
>  307d8c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
>  919b81b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java
>  d320d0f 
>   
> sentry-provider/sent

Re: Review Request 58093: SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw RuntimeExceptio

[Alexander Kolbasov]

> On April 6, 2017, 2:02 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 137 (original), 135 (patched)
> > <https://reviews.apache.org/r/58093/diff/1/?file=1681748#file1681748line141>
> >
> > do you want to print out the value of "retries +1"? Does "retries + 1" 
> > gives you want you want?
> 
> Alexander Kolbasov wrote:
> It does - otherwise I'll see that it failed after 0 retries

I am working on subsequent fix to the same file - will fix it there.


- Alexander


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


On March 31, 2017, 5:07 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58093/
> ---
> 
> (Updated March 31, 2017, 5:07 a.m.)
> 
> 
> Review request for sentry, Lei Xu, Hao Hao, kalyan kumar kalvagadda, and Na 
> Li.
> 
> 
> Bugs: SENTRY-1676
> https://issues.apache.org/jira/browse/SENTRY-1676
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1676: FullUpdateInitializer#createInitialUpdate should not throw 
> RuntimeExceptio
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  146cea2b9467ce82b69bbf402933b1aa350bcd46 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
> 
> 
> Diff: https://reviews.apache.org/r/58093/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 31 (original), 32 (patched)
<https://reviews.apache.org/r/58267/#comment244277>

can be final



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 33 (original), 34 (patched)
<https://reviews.apache.org/r/58267/#comment244289>

I think this field should be initialized by constructor, not callers.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 35 (original), 36 (patched)
<https://reviews.apache.org/r/58267/#comment244278>

Instead of Set it can use Iterable



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 61 (original), 64 (patched)
<https://reviews.apache.org/r/58267/#comment244279>

No one is calling this



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 68 (patched)
<https://reviews.apache.org/r/58267/#comment244281>

Naming - this returns a set of paths for the object, so why is it called 
getPathsString?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 69 (patched)
<https://reviews.apache.org/r/58267/#comment244280>

you can be more precise in defining the HashSet:
Set paths = new HashSet<>(this.paths.size());



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Lines 20 (patched)
<https://reviews.apache.org/r/58267/#comment244284>

Please add some comment explaining why we need a wrapper over String



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Lines 24 (patched)
<https://reviews.apache.org/r/58267/#comment244282>

can it be final?



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

This file uses 2 spaces - I think new changes use different identation



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

The usual convention is to avoid wildcard imports.



sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql
Lines 14 (patched)
<https://reviews.apache.org/r/58267/#comment244286>

Here and in other places - why do we create table first and then declare 
primary key as alter table? Can we declare primary key inline?


- Alexander Kolbasov


On April 7, 2017, 5:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> ---
> 
> (Updated April 7, 2017, 5:24 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
> https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Older JDO defination had issues handling multiple paths associtaed with one 
> Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is 
> associated with more than one path.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  56e456eb37df6d0a14402f57dbc400be9b72 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  80

Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

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

Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Vamsee 
Yarlagadda, and Vadim Spector.


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


Repository: sentry


Description
---

SENTRY-1687 FullUpdateInitializer can be more efficient


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 90aaaef0e15306627d7108f12a74a29848055c0b 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 23552c2512902a8500bfacb1c745ca4b10498cc8 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
 f338ce8abddcde128536a0efef77983990325aa6 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
 b5cbea9d295385bb38b912c13903edace04d7589 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
 e643e01a45de77ef7f465d6921c5ae9e7ce925a2 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 16676fb13b0d5015aefe892a6f7e46812ea75124 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 


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


Testing
---

Updated the unit test to test for bigger HMS snapshots


Thanks,

Alexander Kolbasov

Re: Review Request 58281: SENTRY-1643. AutoIncrement ChangeID of MSentryPermChange/MSentryPathChange may be error-prone

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


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

I'd suggest to use barrier to make sure all threads start racing at the 
same time. This will improve chance of getting conflicts.



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

shutdown will cancel all pending jobs - do you need to wait until all are 
done? Otherwise you might miss some updates



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

it may be better to log it rather then print to stdout


- Alexander Kolbasov


On April 10, 2017, 7:54 p.m., Lei Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58281/
> ---
> 
> (Updated April 10, 2017, 7:54 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1643
> https://issues.apache.org/jira/browse/SENTRY-1643
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When it relies on the SQL auto increment primary key as ChangeID, it can not 
> guarentee the consectivity of the IDs, because while each transaction claims 
> new ID from the table counter, the concurrent transactions which were not 
> success would not return the claimed changeIDs to the pool, thus it can not 
> guarateen the consectivity of change IDs.
> 
> This patch changes to use application logic to force the consectivity of IDs.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  a0d34459d7b2f70e863ef6e078401df81381c91b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
>  476fbcb2ad26de23757842111beb12b154e1562b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
>  f590a5296c047e1acedd39a4f2e4f1de98008d32 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  802b9c6cbf8e9ad015e37037b809b58c956de746 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58281/diff/2/
> 
> 
> Testing
> ---
> 
> Add a new test to conurrently insert changes. 
> 
> mvn test -Dtest=TestSentryStore.
> 
> 
> Thanks,
> 
> Lei Xu
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 69 (patched)
<https://reviews.apache.org/r/58267/#comment27>

Please provide javadoc for this new function
And, again, the name of this function seems wrong


- Alexander Kolbasov


On April 10, 2017, 12:44 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> ---
> 
> (Updated April 10, 2017, 12:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
> https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Older JDO defination had issues handling multiple paths associtaed with one 
> Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is 
> associated with more than one path.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  56e456eb37df6d0a14402f57dbc400be9b72 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  802b9c6cbf8e9ad015e37037b809b58c956de746 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql
>  1883626262bf4f4936f156a7ac74365b9b5873df 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql
>  1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql
>  7de9751892a8ff84067f67d542ac58d33e9148d8 
>   
> sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql
>  adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 547bbe8136186658e7fe76ab24934157ea5300ff 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  5376c166659b3e8e373c8c20818f5b1290af90c9 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/2/
> 
> 
> Testing
> ---
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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



Why do you want to actually recreate a new instance of HMSFollower every time 
you start a service? The only thing that's actually needed is to manage 
execution of the service, not the instance.

IMO HMSFollower should just be a singleton which is created once and can get 
scheduled or not depending on the need. You don't have to make it a singleton 
as part of this change (this would be nice though) but actually creating a new 
one every time seems to be not needed.

Since HMSFollower doesn't actually keep state - it is all stored in DB it 
doesn't make much sense to have multiple instances. And since it is runnable, 
adding start/stop methods to it doesn't seem right.

- Alexander Kolbasov


On April 7, 2017, 5:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 7, 2017, 5:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e46aad4223347d8d057188d31efbb68ed8 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db635f639cef91ce675d34d717e6125a0a4d1 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 128 (original), 221 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line230>
> >
> > should retries = i + 1?

Looks like 'retries' is only used for reporting. I didn't change the retry code 
at all - we can handle this as a separate refactoring.


> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 129 (original), 222 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line231>
> >
> > should we use "break" to get out of the for() loop instead of setting 
> > "i = retryStrategyMaxRetries;"? In current implementation, the line 226 
> > "retries = i;" will be called when InterruptedException  is called, then 
> > "retries" will be set as "retryStrategyMaxRetries". That is not correct

The existing code isn't very clear but seems to be doing the job. I have not 
touched any of the retry logic - can we handle this separately, if needed?


> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
> > Line 125 (original), 214 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686937#file1686937line276>
> >
> > can we have the test case that the first time, client returns 
> > exception, but the second time, it returns correct result. This is to test 
> > the retry behavior.

There was no original test for this as well. I agree, this would be good to 
test, I suggest doing this separately from this changeset which is specificaly 
for the snapshot creation logic.


- Alexander


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


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> ---
> 
> (Updated April 10, 2017, 5:42 a.m.)
> 
> 
> Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio 
> Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1687
> https://issues.apache.org/jira/browse/SENTRY-1687
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1687 FullUpdateInitializer can be more efficient
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2512902a8500bfacb1c745ca4b10498cc8 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
>  f338ce8abddcde128536a0efef77983990325aa6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
>  b5cbea9d295385bb38b912c13903edace04d7589 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  e643e01a45de77ef7f465d6921c5ae9e7ce925a2 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb13b0d5015aefe892a6f7e46812ea75124 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  24ab1a8b392f23bc75759733bef7cecd4bc2ac34 
> 
> 
> Diff: https://reviews.apache.org/r/58284/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the unit test to test for bigger HMS snapshots
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 10, 2017, 3:56 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
> > Line 125 (original), 214 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686937#file1686937line276>
> >
> > can we have the test case that the first time, client returns 
> > exception, but the second time, it returns correct result. This is to test 
> > the retry behavior.
> 
> Alexander Kolbasov wrote:
> There was no original test for this as well. I agree, this would be good 
> to test, I suggest doing this separately from this changeset which is 
> specificaly for the snapshot creation logic.

I opened SENTRY-1701 for the improvements of the retry logic and the unit test 
request.


- Alexander


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


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> ---
> 
> (Updated April 10, 2017, 5:42 a.m.)
> 
> 
> Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio 
> Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1687
> https://issues.apache.org/jira/browse/SENTRY-1687
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1687 FullUpdateInitializer can be more efficient
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2512902a8500bfacb1c745ca4b10498cc8 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
>  f338ce8abddcde128536a0efef77983990325aa6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
>  b5cbea9d295385bb38b912c13903edace04d7589 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  e643e01a45de77ef7f465d6921c5ae9e7ce925a2 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb13b0d5015aefe892a6f7e46812ea75124 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  24ab1a8b392f23bc75759733bef7cecd4bc2ac34 
> 
> 
> Diff: https://reviews.apache.org/r/58284/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the unit test to test for bigger HMS snapshots
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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




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

No need to return at the end of the function.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 376 (original), 383 (patched)
<https://reviews.apache.org/r/58221/#comment244663>

As you are fixing this, can you fix this line as well - we know that dbNam 
== null so we shouldn't try to convert it to string.



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

Style - it is better to have field declarations first, followed by methods. 
The close() method is usually somewhere closer to the end but we don't have any 
specific policy on that.



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

Do you ever expect concurrent calls to close() or not?



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

Note that you need to stop the executor first. Otherwise while you are 
calling close() it may run HMSFOllower which isn't good.



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

Can you call awaitTermination() before you called shutdown()?

The doc says:

Blocks until all tasks have completed execution after a shutdown request, 
or the timeout occurs, or the current thread is interrupted, whichever happens 
first.



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

I think this should never happen.



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

Should we call shutdownNow() in this case?



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

Why do you need this?



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

Why do you need this?



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

This is most likely wrong. What does the cancel of the future do? What task 
does it actually cancel given that this is periodic executor? U think you 
should just use shutdown on the executor instead of this.


- Alexander Kolbasov


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 12, 2017, 3:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> > This is most likely wrong. What does the cancel of the future do? What 
> > task does it actually cancel given that this is periodic executor? U think 
> > you should just use shutdown on the executor instead of this.

It seems to be not exactly wrong, but makes things more complicated then they 
should be. You need to shutdown the executor on exit anyway, so why cancel 
scheduling using futures?


- Alexander


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


On April 12, 2017, 3:47 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 12, 2017, 3:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  6c14f5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 350 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380>
> >
> > This is most likely wrong. What does the cancel of the future do? What 
> > task does it actually cancel given that this is periodic executor? U think 
> > you should just use shutdown on the executor instead of this.
> 
> Alexander Kolbasov wrote:
> It seems to be not exactly wrong, but makes things more complicated then 
> they should be. You need to shutdown the executor on exit anyway, so why 
> cancel scheduling using futures?
> 
> Na Li wrote:
> Based on java doc and discussion in 
> http://stackoverflow.com/questions/33022402/stopping-and-removing-a-task-from-scheduledexecutorservice,
>  it seems future.cancel can stop the task from being scheduled in the future. 
> I can run the testing code and verify the behavior of Future.cancel() 
> tomorrow. If this is the case, then cancel future is less overhead then 
> shutting down the ExecutorService. Once ExecutorService is shutdown, it is 
> useless, and we have to create a new ExecutorService in order to use it.

Cancel is useful if we want to stop execution but want to keep the executor for 
later use. Normally (non-test use) we do want to cancel the executor itself - 
we never need to reuse the executor. For tests we can keep it, but then we need 
to have test-only code, and the overhead isn't important.


- Alexander


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


On April 12, 2017, 5:38 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 12, 2017, 5:38 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> > Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
> It is possible. I will add "synchronized" in front of it.

Under what circumstances is it possible? If it is possible, it may not be 
sufficient to add synchronized() - you may need to protect access to the 
variables as well, but I a not convinced that it is possible. Do we run our 
tests in parallel?


> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 96 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96>
> >
> > I think this should never happen.
> 
> Na Li wrote:
> From API, it could be thrown

from the API - yes, but this shouldn't happen in our case, so we should log 
something about the fact that this should never happen.


- Alexander


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


On April 12, 2017, 5:38 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 12, 2017, 5:38 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78>
> >
> > Do you ever expect concurrent calls to close() or not?
> 
> Na Li wrote:
> It is possible. I will add "synchronized" in front of it.
> 
> Alexander Kolbasov wrote:
> Under what circumstances is it possible? If it is possible, it may not be 
> sufficient to add synchronized() - you may need to protect access to the 
> variables as well, but I a not convinced that it is possible. Do we run our 
> tests in parallel?
> 
> Na Li wrote:
> I am not familar with how Sentry service is actually used. 
> Hypathetically, it is possible that several threads hold the same instance 
> and call its close() concurrently.

This should never hapen during regular use - I think this may only happen if 
tests are running in parallel but I guess that it will break many things.


- Alexander


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


On April 13, 2017, 4:47 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 13, 2017, 4:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: [ATTENTION] PMC members

[Alexander Kolbasov]

I am wondering how many people except me are committers but not PMC members?

- Alex

> On Apr 13, 2017, at 3:17 AM, Colm O hEigeartaigh  wrote:
> 
> Hi PMC members of Apache Sentry,
> 
> There is a discussion on the priv...@sentry.apache.org mailing list that
> requires the attention of Sentry PMC members. There appears to be worringly
> few Sentry PMC members who remain active in the project. I'm cc'ing this to
> the dev list in case there are PMC members who have not subscribed to the
> private list.
> 
> Could I ask all PMC members who are still actively monitoring the project
> to let themselves be known on the private list? If you are having a problem
> subscribing to it then drop me a mail directly.
> 
> Colm.
> 
> 
> -- 
> Colm O hEigeartaigh
> 
> Talend Community Coder
> http://coders.talend.com

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 220 (original), 305 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line327>
> >
> > DbTask is returning an empty object mapping, why is this not necessary 
> > with tables?

I should fix this - we should still go over all partitions even if the table 
location is empty.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 361 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line397>
> >
> > do we want to process a DB without a location?

We still want to go over all tables (which we do) but since there is no 
location, there is nothing to emit for the result.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 362 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line398>
> >
> > shouldn't be better to have a singleton for emptyObjectMapping? or we 
> > want different ObjectMapping with empty maps?

Makes sense. Will do.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 314 (original), 415 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line457>
> >
> > why is the 'failOnRetry' not needed anymore? isn't needed?

Failing in this context doesn't make any sense. It could have a value when this 
was part of HMS plugin (even there it is questionable). We are getting state 
from Hive the only result of this setting would be the failure to create the 
initial snapshot and apply subsequent deltas. We can't communicate the failure 
back to hive and at this stage it isn't possible to fix anything.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 317 (original), 416 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line460>
> >
> > are all executors killed when an exception is thrown here? if not, do 
> > we want to kill the current work because of the failure?

The executors are destroyed in the close() method, not here. It is up to the 
caller to call close() even if exception happened.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 419 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line463>
> >
> > should it be good to have a new method for this merging code?

I am not sure that it will be helpful - it is 10 lines of code that's not used 
by anything else. The only thing we'll get is a pretty method name. I'd rather 
not do it.


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
> > Line 99 (original), 100 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686935#file1686935line103>
> >
> > isn't setDefaultSchema a better name? Btw, Is it Scheme or Schema? If 
> > it is for teting, do we want to keep it package-private only?

Will change the name. It can't be package private because the test is in 
different package (sentry-tests/sentry-tests-hive)


> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
> > Lines 118 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686935#file1686935line121>
> >
> > the method javadoc indicates that null is returned if path is 
> > null/empty." but here we're throwing an exception. Which one is true?

Will update comment.


- Alexander


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


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> --

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

entry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 374 (patched)
<https://reviews.apache.org/r/58221/#comment245001>

catch parameter ie is unused



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

The code uses synchronized() on both, so the comment isn't quite right - it 
would handle concurrent calls to start()/stop() - not that it is needed.


- Alexander Kolbasov


On April 13, 2017, 7:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated April 13, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 39 (patched)
<https://reviews.apache.org/r/57901/#comment245111>

Since this is execurted as part of the client, the metric part is tricky - 
it depends on the enclosing service metric system.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 54 (patched)
<https://reviews.apache.org/r/57901/#comment245108>

Why do you need to keep it stateful and remember the transport?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 67 (patched)
<https://reviews.apache.org/r/57901/#comment245107>

Why do you need that? You can just the config object to constructor instead



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 137 (patched)
<https://reviews.apache.org/r/57901/#comment245109>

You can just pass appropriate config object in constructor.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
Lines 210 (patched)
<https://reviews.apache.org/r/57901/#comment245110>

Why don't you return the created transport here?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 121 (original), 59 (patched)
<https://reviews.apache.org/r/57901/#comment245112>

Calling this in a constructor is suspicious since this is an overridable 
method. You can safeky call it from connect()



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 157 (original), 78 (patched)
<https://reviews.apache.org/r/57901/#comment245113>

It would be better to process all these constants during construction - 
they never change so there is no need to do it on every connect


- Alexander Kolbasov


On April 13, 2017, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 13, 2017, 7:34 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 
> 9d180635ca5ed6fb1def4ffdc4addda5a650593e 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 14, 2017, 6:48 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 328 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line356>
> >
> > HmsFollower is AutoClosable. It will call close. You need not 
> > explicitly call close.

The close() will be called if you use try-with-resource construct, but it 
wouldn't be called by itself.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 14, 2017, 4:02 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 280 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line308>
> >
> > If hmsFollower != null you can assume that notification log is enabled. 
> > I feel you don't have to make check it explicitly.

We'll remove notificationLogEnabled soon anyway since we don't support anything 
else.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Line 196 (original), 186 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line198>
> >
> > Can you clarify why it should be started in constructor and why would 
> > tests fail otherwise?
> 
> Na Li wrote:
> the test TestDbSentryOnFailureHookLoading setup calls 
> AbstractTestWithDbProvider.createContext(), which expects the context to be 
> not null after creating SentryService and before calling 
> SentryService.start(). If we don't schedule the cleaner in constructor, we 
> will fail at assumeNotNull(context); if we schedul the cleaner in 
> constructor, it does not fail at assumeNotNull(context); 
> 
> AbstractTestWithDbProvider.createContext()
> {
> ...
> assumeNotNull(context);
> context = AbstractTestWithHiveServer.createContext(properties);
> policyFile
> .setUserGroupMapping(StaticUserGroup.getStaticMapping())
> .write(context.getPolicyFile(), policyFilePath);
> 
> startSentryService();
> ...
> }

That's rather weird - the context is created right after you verify that it is 
not null. How can that work? And how does the cleaner service affects this 
strange assert?


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 368 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line396>
> >
> > 10 seconds seems a bit excessive
> 
> Na Li wrote:
> This is original valued. What value you suggest?
> 
> Na Li wrote:
> Can we address it in another issue when we understand the reason why it 
> is such large number? My experience is that when changing code, it could 
> break at some seemingly unreleated area.

Sure, let's not address it here.


> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 425 (patched)
> > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line454>
> >
> > The code uses synchronized() on both, so the comment isn't quite right 
> > - it would handle concurrent calls to start()/stop() - not that it is 
> > needed.
> 
> Na Li wrote:
> the synchronized is orignal. You mentioned if multiple threads accese it 
> at the same time, since we don't protect variables, it is still not thread 
> safe.
> 
> Na Li wrote:
> do you want me to remove the comment?

It may be better to update it - the current comment contradicts the code.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 160 (patched)
> > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line160>
> >
> > Why do you need this?
> 
> Na Li wrote:
> to track the task that schedules sentryStoreCleaner, so I can cancel it 
> in stop()
> 
> Na Li wrote:
> I still keep it to mark that the cleaner is scheduled if the future is 
> not null.
> 
> The problems are
> 1. We need to create sentryStoreCleanService and schedule cleaner (call 
> startSentryStoreCleaner()) in runServer() if SentryService.start() is called 
> after SentryService.stop(), which shutdown sentryStoreCleanService. 
> Otherwise, the cleaner cannot be scheduled any more. Test may fail.
> 2. We need to create sentryStoreCleanService and schedule cleaner in 
> SentryService constructor (call startSentryStoreCleaner()), so the test 
> TestDbSentryOnFailureHookLoading won't fail. It expects the context to be not 
> null in createContext() before calling SentryService.start(). 
> 3. To satify item 1. and 2., we need to call startSentryStoreCleaner() at 
> both constructor and runServer(). To avoid scheduling the cleaner twice, we 
> need to detect if it is scheduled already.
> 4. When I tried to fix test failures, it seems it is better to create the 
> sentryStoreCleanService at the beginning of creating SentryService. Then 
> checking if sentryStoreCleanService is null or not cannot detect if the 
> cleaner is scheduled already.
> 
> 
> When the SentryService is created, we want to create 
> sentryStoreCleanService before calling startSentryStoreCleaner(). But after 
> stop(), we need to create sentryStoreCleanService within 
> startSentryStoreCleaner. So I choose to use sentryStoreCleanerFuture to 
> decide if we should schedule the cleaner

Thanks for the explanation. This looks like complete mess. You don't have to 
untangle it in this JIRA, but please

1) Add a comment with the above explanation
2) File a follow-up JIRA for cleaning this mess up.


- Alexander


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


On April 14, 2017, 8:36 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 14, 2017, 8:36 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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




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

Please add comment explaining how do you use the future.



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

Please add clarifying parenthesis:

if (notificationLogEnabled && (hmsFollowerExecutor == null) && (hmsFollower 
!= null)) {



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

We can't handle this - if HMSFollowert should start and it doesn't, 
SentryService should fail. The easiest thing may be just to propagate the 
exception up the chain.



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

You can move this outside the try block and return immediately if 
storeCleanPeriodSecs <= 0. This will simplify the code a bit.


- Alexander Kolbasov


On April 17, 2017, 5:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 17, 2017, 5:55 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/12/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

> On April 15, 2017, 12:36 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
> > Lines 67 (patched)
> > <https://reviews.apache.org/r/57901/diff/4/?file=1692051#file1692051line67>
> >
> > Why do you need that? You can just the config object to constructor 
> > instead
> 
> kalyan kumar kalvagadda wrote:
> I want to abstract the config implementation to sentry-core package. I 
> don't see a need for thc client's be aware of it. All they have to say is the 
> type of the client it is and is handled internally.

The reason you provided the nice interface for the configuration is that no one 
needs to know the type of the client, they need to have the implementation of 
the correct configuration passed to them. You abstract this by passing an 
interface, Going back to creating a specific configuration there is breaking 
the abstraction. Why does the transport generator need to know specific type of 
client it is dealing with? It doesn't care!


- Alexander


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


On April 13, 2017, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -------
> 
> (Updated April 13, 2017, 7:34 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 
> 9d180635ca5ed6fb1def4ffdc4addda5a650593e 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  4ed1361fb1d79620c3098355a0ea9c27203a75de 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2512902a8500bfacb1c745ca4b10498cc8 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml 
> beda2029616538101dc368b3d272fdfc942868ad 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java
>  085971b34e3901b7a1d59bd8e7516b25f81ca872 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java
>  ab12bf402a9a04b28e2c6c06d4e55a3607132ead 
>   
> sentry-hdfs/sentry-hdfs-ser

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

> On April 15, 2017, 12:36 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
> > Lines 210 (patched)
> > <https://reviews.apache.org/r/57901/diff/4/?file=1692051#file1692051line210>
> >
> > Why don't you return the created transport here?
> 
> kalyan kumar kalvagadda wrote:
> SentryTransport handles the transport for the sentry clients.

What do you mean by "handle"? It doesn't need to hold on to it! Consumers can 
keep the open transports themselves - there is no need to keep any state here.


> On April 15, 2017, 12:36 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 121 (original), 59 (patched)
> > <https://reviews.apache.org/r/57901/diff/4/?file=1692057#file1692057line123>
> >
> > Calling this in a constructor is suspicious since this is an 
> > overridable method. You can safeky call it from connect()
> 
> kalyan kumar kalvagadda wrote:
> I don't want the connection creation delayed till the client RPC is 
> invoked. That is what i'm trying to connect to sentry server when the client 
> is constructed.

There is very little delay in practice so this doesn't buy you much. I think 
connect() should be separate from construction - it is like socket open() and 
connect() - they are separate operations.


- Alexander


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


On April 13, 2017, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 13, 2017, 7:34 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 
> 9d180635ca5ed6fb1def4ffdc4addda5a650593e 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  4ed1361fb1d79620c3098355a0ea9c27203a75de 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
&g

Re: Review Request 58412: [WIP]SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change

[Alexander Kolbasov]

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




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

This doesn't look right - there may be duplicate paths in which case this 
will fail. You can find a path for the specific object, but not globally



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

it may be better to quote the event - e.g.

Processes "create database" notification event. Otherwise it reads as if 
processes create some events.

The same comments applies for other methods.


- Alexander Kolbasov


On April 13, 2017, 6:58 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> ---
> 
> (Updated April 13, 2017, 6:58 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> [WIP]SENTRY-1587: Refactor SentryStore transaction to persist a single path 
> transcation bundled with corresponding delta path change
> 
> Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c22364fa26c80415827313f4d26f6c53b71b6f6c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  aaea9790282f9136302eec64107cc86391a4d6ff 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb13b0d5015aefe892a6f7e46812ea75124 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  31a309bdc789bd7a997f7654e30f2021ecb5b616 
> 
> 
> Diff: https://reviews.apache.org/r/58412/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 58412: [WIP]SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change

[Alexander Kolbasov]

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




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

I don't think you need to explicitely do this - it should be sufficient to 
remove the path from the set and update the MAuthzPathsMapping object.



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

Is this used?


- Alexander Kolbasov


On April 13, 2017, 6:58 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> ---
> 
> (Updated April 13, 2017, 6:58 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> [WIP]SENTRY-1587: Refactor SentryStore transaction to persist a single path 
> transcation bundled with corresponding delta path change
> 
> Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c22364fa26c80415827313f4d26f6c53b71b6f6c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  aaea9790282f9136302eec64107cc86391a4d6ff 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb13b0d5015aefe892a6f7e46812ea75124 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  31a309bdc789bd7a997f7654e30f2021ecb5b616 
> 
> 
> Diff: https://reviews.apache.org/r/58412/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

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




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

Please add comment explaining what this is



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

size() may be expensive operation for some collection - it is better to use 
isEmpty().

Also, if there are no events we are not going to process any new ones, so 
does it make sense to log?

I would suggest

if ((currentEventID != lastLoggedEventId) && 
response.getEvents().isEmpty()) {



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 257 (original), 261 (patched)
<https://reviews.apache.org/r/58481/#comment245245>

I think this should be debug() - there is no need to log every update.


- Alexander Kolbasov


On April 17, 2017, 5:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 17, 2017, 5:56 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

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



- Alexander Kolbasov


On April 17, 2017, 5:56 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 17, 2017, 5:56 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

> On April 13, 2017, 7:13 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Line 220 (original), 305 (patched)
> > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line327>
> >
> > DbTask is returning an empty object mapping, why is this not necessary 
> > with tables?
> 
> Alexander Kolbasov wrote:
> I should fix this - we should still go over all partitions even if the 
> table location is empty.

Can you clarify the question - For DB task we return an empty map. For Table 
task we do not emit any empty paths, so we only put entries for existing paths. 
What is your concern here?


- Alexander


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


On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58284/
> ---
> 
> (Updated April 10, 2017, 5:42 a.m.)
> 
> 
> Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio 
> Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1687
> https://issues.apache.org/jira/browse/SENTRY-1687
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1687 FullUpdateInitializer can be more efficient
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2512902a8500bfacb1c745ca4b10498cc8 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
>  f338ce8abddcde128536a0efef77983990325aa6 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
>  b5cbea9d295385bb38b912c13903edace04d7589 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
>  e643e01a45de77ef7f465d6921c5ae9e7ce925a2 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb13b0d5015aefe892a6f7e46812ea75124 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  24ab1a8b392f23bc75759733bef7cecd4bc2ac34 
> 
> 
> Diff: https://reviews.apache.org/r/58284/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the unit test to test for bigger HMS snapshots
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

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

(Updated April 18, 2017, 12:26 a.m.)


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


Changes
---

Addressed code reveiw comments


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


Repository: sentry


Description
---

SENTRY-1687 FullUpdateInitializer can be more efficient


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 90aaaef0e15306627d7108f12a74a29848055c0b 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 23552c2512902a8500bfacb1c745ca4b10498cc8 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
 f338ce8abddcde128536a0efef77983990325aa6 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
 b5cbea9d295385bb38b912c13903edace04d7589 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
 e643e01a45de77ef7f465d6921c5ae9e7ce925a2 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 16676fb13b0d5015aefe892a6f7e46812ea75124 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 


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

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


Testing
---

Updated the unit test to test for bigger HMS snapshots


Thanks,

Alexander Kolbasov

[DISCUSS] Sentry and Hive versions

[Alexander Kolbasov]

I am curious whether we have any specific requirements for Hive versions
that should be supported by Sentry?

The question comes from the fact that Sentry HA relies on the Hive
notification mechanism introduced in Hive 1.2 and some improvements to it
made in Hive 1.3. If we base Sentry 1.8 on Sentry HA, we may require Hive
1.3 or above. Would this be an issue or not?

- Alex

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 32 (patched)
<https://reviews.apache.org/r/57901/#comment245267>

The comment should explain what the method does which is 

"Open a connection to an instance of Sentry Server".



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 43 (patched)
<https://reviews.apache.org/r/57901/#comment245268>

Javadoc?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 51 (original), 40 (patched)
<https://reviews.apache.org/r/57901/#comment245269>

Wrong formatting. This should be a proper javadoc.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 56 (original), 44 (patched)
<https://reviews.apache.org/r/57901/#comment245270>

Is there any real concurrent access to it - and the need to be synchronized?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 73 (patched)
<https://reviews.apache.org/r/57901/#comment245271>

What do you get by separating createThriftCLient into a separate function?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 171 (original), 92 (patched)
<https://reviews.apache.org/r/57901/#comment245272>

Add @Oberride



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 180 (original), 101 (patched)
<https://reviews.apache.org/r/57901/#comment245273>

Add @Override



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 189 (original), 110 (patched)
<https://reviews.apache.org/r/57901/#comment245274>

Add @Override



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 210 (original), 131 (patched)
<https://reviews.apache.org/r/57901/#comment245275>

Add @Override



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 135 (patched)
<https://reviews.apache.org/r/57901/#comment245276>

Add @Override



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
Line 44 (original), 43 (patched)
<https://reviews.apache.org/r/57901/#comment245277>

Is this comment still valid?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
Line 81 (original), 80 (patched)
<https://reviews.apache.org/r/57901/#comment245278>

missorted synchronzied public - should be public synchronzied


- Alexander Kolbasov


On April 13, 2017, 7:34 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -------
> 
> (Updated April 13, 2017, 7:34 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 
> 9d180635ca5e

Re: [DISCUSS] Merging Sentry HA branch with master

[Alexander Kolbasov]

FYI, I contacted Colin Ma who was working on the refactoring and he was Ok
with skipping this change for 1.8. That said, the refactoring was done for
a reason it we should get back to it post 1.8,

- Alex

On Mon, Apr 17, 2017 at 5:51 PM Hao Hao  wrote:

> The proposal is good to me as well. Should we start a vote on this? Or we
> can just start to do it if no one is objecting?
>
> Best,
> Hao
>
> On Mon, Apr 17, 2017 at 4:42 PM, Vamsee Yarlagadda 
> wrote:
>
> > >
> > > Cherry-pick any commits that happened since sentry-ha-redesign was
> > forked,
> > > except a few described below
> > > Exclude big refactoring commit (SENTRY-1205) and related commits
> > > (SENTRY-1436, SENTRY-1438, SENTRY-1406)
> > > Rename master to a dev branch
> > > Rename sentry-ha-redesign to master
> >
> >
> > This sounds good to me. Generally having merge commits complicates the
> git
> > history and might get tricky when we are debugging things. I would rather
> > stick with the approach of cherry-picks to make the history clear.
> >
> > Thanks,
> > Vamsee
> >
> > On Tue, Apr 4, 2017 at 7:17 PM, Alexander Kolbasov 
> > wrote:
> >
> > > I would like to make a more concrete proposal and I am interested in
> > > opinion from Sentry PMC members on this.
> > >
> > > I would propose the following approach for merging Sentry HA into
> Sentry
> > > master:
> > >
> > > Cherry-pick any commits that happened since sentry-ha-redesign was
> > forked,
> > > except a few described below
> > > Exclude big refactoring commit (SENTRY-1205) and related commits
> > > (SENTRY-1436, SENTRY-1438, SENTRY-1406)
> > > Rename master to a dev branch
> > > Rename sentry-ha-redesign to master
> > >
> > > What does community think about such approach?
> > >
> > > - Alex
> > >
> > >
> > > > On Mar 22, 2017, at 1:43 PM, Alexander Kolbasov 
> > > wrote:
> > > >
> > > > Hello,
> > > >
> > > > I would like to start the discussion on merging sentry-ha-redesign
> > > branch with master.
> > > >
> > > > As of now most of the changes from master are merged into
> > > sentry-ha-redesign. The major missing part is SENTRY-1205 (Refactor the
> > > code for sentry-provider-db and create sentry-service module) and
> > > associated issues. This refactoring is very hard to port, especially
> > since
> > > there is very little information in the JIRA on why it was done and how
> > it
> > > was done - was it merely moving files around or more then that. I would
> > > seriously consider not including this change in 1.8.
> > > >
> > > > So in regards to the merge we have several options:
> > > >
> > > > Attempt to merge master into sentry-ha-redesign, resolve any
> conflicts
> > > and later commit the merge to master. This will cause merge commit on
> > master
> > > > Finish work on sentry-ha-redesign, make sure that relevant commits
> are
> > > ported from master, and then making this a master branch and making
> > current
> > > master a special branch left for reference purposes. This will likely
> > leave
> > > SENTRY-1205 and related issues out.
> > > > What does community think about this?
> > > >
> > > > - Alex
> > >
> > >
> >
> >
> > --
> > Thanks,
> > Vamsee
> >
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

> On April 17, 2017, 10:42 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 258 (patched)
> > <https://reviews.apache.org/r/58481/diff/2/?file=1693243#file1693243line258>
> >
> > size() may be expensive operation for some collection - it is better to 
> > use isEmpty().
> > 
> > Also, if there are no events we are not going to process any new ones, 
> > so does it make sense to log?
> > 
> > I would suggest
> > 
> > if ((currentEventID != lastLoggedEventId) && 
> > response.getEvents().isEmpty()) {
> 
> Na Li wrote:
> in my approach, for example, the log messages will be
> 
> CurrentEventId = 2, Processing 2 events
> CurrentEventId = 4, processing 0 events
> CurrentEventId = 4, processing 3 events
> 
> in your approach, the log messages will be
> CurrentEventId = 2, Processing 2 events
> CurrentEventId = 4, processing 3 events
> 
> With this output, we don't need to track lastLoggedEventId. When the set 
> is not empty, the ID will increase in normal case. If it stays the same, 
> something is wrong, and we should log it.
> 
> Na Li wrote:
> I use a variable to remember the value of response.getEvents().size(). 
> based on 
> http://stackoverflow.com/questions/24392682/java-list-size-performance-and-tips,
>  the overhead of List.size() is O(1).

I've read somewhere that for linked lists it isn't true but other docs suggest 
that it is still O(1), so you are right. IMO isEmpty() more clearly shows the 
intention, but I drop my perf argument.

As for the the two approaches - it seems reasonable both ways so please do 
whatever you think is best.


- Alexander


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


On April 18, 2017, 4:29 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 18, 2017, 4:29 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

> On April 17, 2017, 10:42 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 258 (patched)
> > <https://reviews.apache.org/r/58481/diff/2/?file=1693243#file1693243line258>
> >
> > size() may be expensive operation for some collection - it is better to 
> > use isEmpty().
> > 
> > Also, if there are no events we are not going to process any new ones, 
> > so does it make sense to log?
> > 
> > I would suggest
> > 
> > if ((currentEventID != lastLoggedEventId) && 
> > response.getEvents().isEmpty()) {
> 
> Na Li wrote:
> in my approach, for example, the log messages will be
> 
> CurrentEventId = 2, Processing 2 events
> CurrentEventId = 4, processing 0 events
> CurrentEventId = 4, processing 3 events
> 
> in your approach, the log messages will be
> CurrentEventId = 2, Processing 2 events
> CurrentEventId = 4, processing 3 events
> 
> With this output, we don't need to track lastLoggedEventId. When the set 
> is not empty, the ID will increase in normal case. If it stays the same, 
> something is wrong, and we should log it.
> 
> Na Li wrote:
> I use a variable to remember the value of response.getEvents().size(). 
> based on 
> http://stackoverflow.com/questions/24392682/java-list-size-performance-and-tips,
>  the overhead of List.size() is O(1).
> 
> Alexander Kolbasov wrote:
> I've read somewhere that for linked lists it isn't true but other docs 
> suggest that it is still O(1), so you are right. IMO isEmpty() more clearly 
> shows the intention, but I drop my perf argument.
> 
> As for the the two approaches - it seems reasonable both ways so please 
> do whatever you think is best.

What will happen if for some reason HMSFOllower isn't processing updates? In 
responses would be non-empty and it will always print log the message on every 
invocation which isn't what we want


- Alexander


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


On April 18, 2017, 4:29 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 18, 2017, 4:29 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 18, 2017, 4:29 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 18, 2017, 4:29 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58412: SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change

[Alexander Kolbasov]

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



Initial batch of review comments.


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

It would be nice to explain the problem in HIVE-15761 in a few words and 
how this works around it.



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

Would this work correctly if we have two HMSFollowers running concurrently?



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

I would print stacktrace here as well.



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

Here and in other places in this function there isn't any value in throwing 
an exception. We got something wrong from Hive, we should just log it and 
continue. Stack trace would add much value.

Also here we know that one of the three is null and yet we try to show them 
all as %s so we are likely to get NPE here instead.



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

ame comment about exception.



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

Same comment about exception.



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

I'd suggest rewriting as

if (! sync...) {
  return
}



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

Same suggestion for early return



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 56 (patched)
<https://reviews.apache.org/r/58412/#comment245405>

Here and in other similar places, you may consider using 
Collections.singletonList(location). If you don't want to use it, consider 
creating ArrayList with size 1.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 88 (patched)
<https://reviews.apache.org/r/58412/#comment245406>

You convert authzObj to lower case in addPaths anyway, so this isn't needed



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 176 (patched)
<https://reviews.apache.org/r/58412/#comment245408>

seems that newAuthzObj is redundand.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 182 (patched)
<https://reviews.apache.org/r/58412/#comment245407>

you may want to remove dot on the previous line.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 195 (patched)
<https://reviews.apache.org/r/58412/#comment245411>

new HashSet<>()



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 221 (patched)
<https://reviews.apache.org/r/58412/#comment245410>

remove dot on the previous line



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 234 (patched)
<https://reviews.apache.org/r/58412/#comment245409>

It is better to use new HashSet<>() here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 243 (patched)
<https://reviews.apache.org/r/58412/#comment245412>

Do you want to continue with other paths instead of returning here? Table 
may contan some partitions which do not have valid HDFS path.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 334 (patched)
<https://reviews.apache.org/r/58412/#comment245413>

Is there anything to do if oldPathTree is null but newPathTree is not null?


- Alexander Kolbasov


On April 18, 2017, 9:34 p.m., Hao Hao wrote:
> 
> --

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 259 (original), 257 (patched)
<https://reviews.apache.org/r/58481/#comment245484>

This still preserves the original problem when events are not processed for 
some reason - you will get a flood of logs. In this case events would be always 
non-empty.



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

This comment no longer matches the code.


- Alexander Kolbasov


On April 19, 2017, 2:43 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 19, 2017, 2:43 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: [DISCUSS] Merging Sentry HA branch with master

[Alexander Kolbasov]

Sergio,

thank you for clarifying the Hive version story.

Do we have any explicit policy about compatibility with other components and 
Hive specifically? Do we need to support older versions of Hive or supporting 
the current released Hive 1.x version is sufficient?

At least it seems that we need to address 
SENTRY-1323 Bump the hive version to 1.2.0

to move forward.

- Alex

> On Apr 19, 2017, at 8:34 AM, Sergio Pena  wrote:
> 
> Pointing the branch to master sounds good.
> 
> I just have one concern regarding the Hive dependency that Sentry HA needs.
> This new feature requires that the Hive version running with Sentry master
> has HMS notification logs enabled. Hive 1.1.0 (currently officially
> supported by Sentry 1.7) started this HMS notification feature but seems it
> is unstable. At least I found a bug on Hive 1.1.0 that it might make Sentry
> unusable with this version + others required fixes and improvements that
> exist only on Hive 2.x.
> 
> This bug is fixed on Hive 1.2.0:
>   HIVE-9501: DbNotificationListener doesn't include dbname in create
> database notification and does not include tablename in create table
> notification.
> 
> To make things complicated, bumping Sentry to support Hive 1.2.0 is not an
> easy task due to the new authorization v2 hooks that are used in Hive. See
> the comments on the below SENTRY jira:
> 
>   SENTRY-1323: Bump the hive version to 1.2.0
> 
> So, my question is what would happen with the Sentry 1.x compatibility if
> we do this switch from sentry-ha-redesign -> master?
> 
> On Tue, Apr 18, 2017 at 3:45 AM, Colm O hEigeartaigh 
> wrote:
> 
>> OK thanks, sounds good to me then. Let's try and do development work on
>> master from now on though...
>> 
>> Colm.
>> 
>> On Tue, Apr 18, 2017 at 2:14 AM, Alexander Kolbasov 
>> wrote:
>> 
>>> FYI, I contacted Colin Ma who was working on the refactoring and he was
>> Ok
>>> with skipping this change for 1.8. That said, the refactoring was done
>> for
>>> a reason it we should get back to it post 1.8,
>>> 
>>> - Alex
>>> 
>>> On Mon, Apr 17, 2017 at 5:51 PM Hao Hao  wrote:
>>> 
>>>> The proposal is good to me as well. Should we start a vote on this? Or
>> we
>>>> can just start to do it if no one is objecting?
>>>> 
>>>> Best,
>>>> Hao
>>>> 
>>>> On Mon, Apr 17, 2017 at 4:42 PM, Vamsee Yarlagadda <
>> vam...@cloudera.com>
>>>> wrote:
>>>> 
>>>>>> 
>>>>>> Cherry-pick any commits that happened since sentry-ha-redesign was
>>>>> forked,
>>>>>> except a few described below
>>>>>> Exclude big refactoring commit (SENTRY-1205) and related commits
>>>>>> (SENTRY-1436, SENTRY-1438, SENTRY-1406)
>>>>>> Rename master to a dev branch
>>>>>> Rename sentry-ha-redesign to master
>>>>> 
>>>>> 
>>>>> This sounds good to me. Generally having merge commits complicates
>> the
>>>> git
>>>>> history and might get tricky when we are debugging things. I would
>>> rather
>>>>> stick with the approach of cherry-picks to make the history clear.
>>>>> 
>>>>> Thanks,
>>>>> Vamsee
>>>>> 
>>>>> On Tue, Apr 4, 2017 at 7:17 PM, Alexander Kolbasov <
>> ak...@cloudera.com
>>>> 
>>>>> wrote:
>>>>> 
>>>>>> I would like to make a more concrete proposal and I am interested
>> in
>>>>>> opinion from Sentry PMC members on this.
>>>>>> 
>>>>>> I would propose the following approach for merging Sentry HA into
>>>> Sentry
>>>>>> master:
>>>>>> 
>>>>>> Cherry-pick any commits that happened since sentry-ha-redesign was
>>>>> forked,
>>>>>> except a few described below
>>>>>> Exclude big refactoring commit (SENTRY-1205) and related commits
>>>>>> (SENTRY-1436, SENTRY-1438, SENTRY-1406)
>>>>>> Rename master to a dev branch
>>>>>> Rename sentry-ha-redesign to master
>>>>>> 
>>>>>> What does community think about such approach?
>>>>>> 
>>>>>> - Alex
>>>>>> 
>>>>>> 
>>>>>>> On Mar 22, 2017, at 1:43 PM, Alexander Kolbasov <
>>> ak...@cloudera.com>
>>>>>

Clarification for MPath changes for SENTRY-1587

[Alexander Kolbasov]

Hao,

Can you clarify the changes you propose for MPath class and related
package.jdo changes for SENTRY-1587?

You suggest changing the identity type from "database" to "application",
but the pathId is not initialized in constructor and not assigned anywhere.
What is your intention here?

Also you changed the equals() method to include ID. What kind of equality
do you require - equality by path value or equality by identity? What is
your thinking here?

This is in regards to the code review https://reviews.apache.org/r/58412.

Thanks,

- Alex

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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




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

I think you may need to wait for termination of HMSFOllower here. Otherwice 
it may be still running when you call close() and this may cause troubles.


- Alexander Kolbasov


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 221 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line249>
> >
> > SentryStore cleaner need not be restarted every time sentry service is 
> > stoped and started.
> 
> Na Li wrote:
> in normal operation, stop() is the last call to clean up the resources. 
> And we need to stop SentryStore cleaner.

Normally we don't know whether this is the stop() happening because we are 
exiting or this is a test. During exit we do need to shut down the executor 
service. There is no harm in restarting the service for tests.


- Alexander


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On April 14, 2017, 7:04 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/58221/diff/10/?file=1692735#file1692735line250>
> >
> > I feel we should start call startHMSFollower at the end of runServer 
> > function. If there are no issues in starting the sentry service, we can 
> > start HMSFolloeer at the end.
> 
> Na Li wrote:
> The original code to start HMSFollower is in constructor. When I move it 
> to runServer(), putting it at the beginning is the closest approach to the 
> original in terms of the ordering. I am not familar with the system, and 
> moving it to the end of runServer() may cause some unknown problems.
> 
> Na Li wrote:
> I will move it to the end of runServer()
> 
> Na Li wrote:
> there is regression and test 
> org.apache.sentry.tests.e2e.metastore.TestDbNotificationListenerSentryDeserializer.testAlterPartition
>  failed after moving the function to the end of runServer(). So I move it 
> back to the beginning of runServer().

Why do you think that it is better to start HMSFollower in the end?


- Alexander


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


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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




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

It seems that there is a valid use case for tests to call start() with 
diferent configurations to start new HMSFollower in start() method - this way, 
for example, tests may start it with different metastore URI. This contradicts 
my earlier comment, but I didn't know about this use case.

So I would consider managing HMSFollower here as well.


- Alexander Kolbasov


On April 19, 2017, 2:23 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 19, 2017, 2:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/17/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Clarification for MPath changes for SENTRY-1587

[Alexander Kolbasov]

Lina,

Note that there my be multiple path values, but only a single path value for a 
given object since paths are stored as a set. The guarantee is provided by set 
semantics. We do not particularly care about duplicates across multiple objects.

- Alex.

> On Apr 19, 2017, at 1:09 PM, Na Li  wrote:
> 
> Sasha,
> 
> For now, the path entry in DB is owned by each Authz object. 
> 
> The DB change Kalyan made does not guarantee that with the same path value, 
> there is only one instance of such path entry in DB in 
> https://issues.apache.org/jira/browse/SENTRY-1629 
> <https://issues.apache.org/jira/browse/SENTRY-1629> (He will address this 
> later). Therefore, there could be two or more entries that have the same path 
> value. So Hao has to change the equals() to find the path instance owned by 
> authz Object.
> 
> 
> 
> Thanks,
> 
> Lina
> 
> On Wed, Apr 19, 2017 at 2:05 PM, Hao Hao  <mailto:hao@cloudera.com>> wrote:
> Hi Alex,
> 
> I changed MPath class to include pathID to have correct MPath equality
> comparison. Since the original equals() definition only compares 'path'
> which for different MPath object it may have the same value.
> 
> For pathID (primary key) assignment I change it to be auto-increment by
> specifying value-strategy to be "increment ".
> 
> Please let me know if you think any of the change is not reasonable. Thanks!
> 
> Best,
> Hao
> 
> On Wed, Apr 19, 2017 at 11:37 AM, Alexander Kolbasov  <mailto:ak...@cloudera.com>>
> wrote:
> 
> > Hao,
> >
> > Can you clarify the changes you propose for MPath class and related
> > package.jdo changes for SENTRY-1587?
> >
> > You suggest changing the identity type from "database" to "application",
> > but the pathId is not initialized in constructor and not assigned anywhere.
> > What is your intention here?
> >
> > Also you changed the equals() method to include ID. What kind of equality
> > do you require - equality by path value or equality by identity? What is
> > your thinking here?
> >
> > This is in regards to the code review https://reviews.apache.org/r/58412 
> > <https://reviews.apache.org/r/58412>.
> >
> > Thanks,
> >
> > - Alex
> >
> 

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 20, 2017, 12:17 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 20, 2017, 12:17 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/18/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58412: SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Lines 27 (patched)
<https://reviews.apache.org/r/58412/#comment245494>

Please add a comment explaining why we need application identity in this 
case - it isn't obvious.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Line 63 (original), 69 (patched)
<https://reviews.apache.org/r/58412/#comment245493>

We can't have two objects with the same ID but different paths, so we can 
skip testing for actual path equality if we are using ID equality.



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

It never throws such exception



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

It never throws such exception



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

Please do not overload the name - it is very confusing



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

    Please do not overload the name


- Alexander Kolbasov


On April 18, 2017, 9:34 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> ---
> 
> (Updated April 18, 2017, 9:34 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1587: Refactor SentryStore transaction to persist a single path 
> transcation bundled with corresponding delta path change
> 
> Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c22364fa26c80415827313f4d26f6c53b71b6f6c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  c74384688ca920c79fb2987d225042e135cdfd53 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  08520f30f3529af88d7dab9ae3623f28f0e43558 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb13b0d5015aefe892a6f7e46812ea75124 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  87e329588990be129197d598dd1db4487f8e0f25 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  24ab1a8b392f23bc75759733bef7cecd4bc2ac34 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  7769f24bb4c062016084bcafe7d50a0f0e013824 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  0e97466c9ba6973d8e787819e292c3b826472b38 
> 
> 
> Diff: https://reviews.apache.org/r/58412/diff/3/
> 
> 
> Testing
> ---
> 
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

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




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

I don't think we need this



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

We log failures to process events when we detect these, so is there a real 
need to log it here as well?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 259 (original), 278 (patched)
<https://reviews.apache.org/r/58481/#comment245655>

There is no point in calling this if there are no events


- Alexander Kolbasov


On April 20, 2017, 6:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 20, 2017, 6:14 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58593: SENTRY-1710. Reduce datanucleus key cache size for MSentryPermChange and MSentryPathChange tables to avoid holes in change IDs.

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 20, 2017, 8:56 p.m., Lei Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58593/
> ---
> 
> (Updated April 20, 2017, 8:56 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Na Li.
> 
> 
> Bugs: SENTRY-1710
> https://issues.apache.org/jira/browse/SENTRY-1710
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> As discussed in SENTRY-1643 and SENTRY-1706, MSentryPermChange and 
> MSentryPathChange tables require that CHANGE ID of such changes to be unique 
> and consecutive. SENTRY-1644 addressed this requirement in the application 
> code, that requires multiple trips for each change record insertion, while it 
> raises the possibility of collision on change ID when there are concurrently 
> updates.
> 
> This patch change the ```key-cache-size``` to 1, as Lina suggested.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java
>  4b42ed02d7b6996ee9e460f346ebfefc44be9734 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java
>  a97d10ad92739ed17e543372024b6afb652fe0be 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java
>  8d3c88ba24eef76c6c07d06fb426e27b1d7d7f6b 
> 
> 
> Diff: https://reviews.apache.org/r/58593/diff/1/
> 
> 
> Testing
> ---
> 
> mvn test -Dtest=TestSentryStore
> 
> 
> Thanks,
> 
> Lei Xu
> 
>

Re: Review Request 58481: SENTRY-1674 HMSFollower shouldn't print the same value of notification ID multiple times

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 20, 2017, 11:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58481/
> ---
> 
> (Updated April 20, 2017, 11:55 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1674
> https://issues.apache.org/jira/browse/SENTRY-1674
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not log message when CurrentEventID does not change and there is no 
> updates for HMSFollower
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  16676fb 
> 
> 
> Diff: https://reviews.apache.org/r/58481/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58412: SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 21, 2017, 2:20 a.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> ---
> 
> (Updated April 21, 2017, 2:20 a.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1587: Refactor SentryStore transaction to persist a single path 
> transcation bundled with corresponding delta path change
> 
> Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
>  90aaaef0e15306627d7108f12a74a29848055c0b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  14e967aa1065f16e8d4c3f61db2f9055959fa9e6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c22364fa26c80415827313f4d26f6c53b71b6f6c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  08520f30f3529af88d7dab9ae3623f28f0e43558 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  1543379eec2403fd1fbe61947c4c38a189177895 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  87e329588990be129197d598dd1db4487f8e0f25 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
>  24ab1a8b392f23bc75759733bef7cecd4bc2ac34 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  7769f24bb4c062016084bcafe7d50a0f0e013824 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  0e97466c9ba6973d8e787819e292c3b826472b38 
> 
> 
> Diff: https://reviews.apache.org/r/58412/diff/4/
> 
> 
> Testing
> ---
> 
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 58631: SENTRY-1691: TransactionManager should use try-with-resource for timers

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 21, 2017, 9:22 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58631/
> ---
> 
> (Updated April 21, 2017, 9:22 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1691
> https://issues.apache.org/jira/browse/SENTRY-1691
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Moved the timer() inside the try-with-resources.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java
>  fb7c40a4d32f6c5588b277c637e041ca8b3d999e 
> 
> 
> Diff: https://reviews.apache.org/r/58631/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Question about on-demand Sentry build

[Alexander Kolbasov]

Hello,

I discovered the build-on-demand page for Jenkins build:

https://builds.apache.org/view/PreCommit%20Builds/job/PreCommit-SENTRY-Build-On-Demand/

When I tried it I discovered that it looks at the old version of Sentry at
https://git-wip-us.apache.org/repos/asf/incubator-sentry.git.

I think it would be useful to resurrect this job - does anyone know how to
update the configuration?

Thanks,

- Alex

Stuck Sentry builds?

[Alexander Kolbasov]

It looks like updates to JIRA stopped triggering builds as  today morning.
Does anyone know how to debug this?

Thanks,

- Alex

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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




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

What if (hmsFollowerExecutor == null && hmsFollower != null) ?

In this case you are going to override your existing HMSFollower with a new 
one. This should never happen though because you set it to null in close.

So I think you can simplify the check and if you want, you can add a 
precondition that hmsFollower is null.



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

We are doing this twice - it is worth putting it in a helper function.


- Alexander Kolbasov


On April 24, 2017, 8:43 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated April 24, 2017, 8:43 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  57b7f88 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/20/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58284: SENTRY-1687 FullUpdateInitializer can be more efficient

[Alexander Kolbasov]

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

(Updated April 26, 2017, 6:51 a.m.)


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


Changes
---

merged with SENTRY-1587 changes.
This involved changes to Notification processor in the way it handles 
PathsUpdate.parsePath()


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


Repository: sentry


Description
---

SENTRY-1687 FullUpdateInitializer can be more efficient


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
 d876d230f3200c71e9d11681efce8b24cac40b7e 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
 e32d4a720e49464f812b12c46e8e0e040cb36fad 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 23552c2512902a8500bfacb1c745ca4b10498cc8 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java
 f338ce8abddcde128536a0efef77983990325aa6 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java
 b5cbea9d295385bb38b912c13903edace04d7589 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
 e643e01a45de77ef7f465d6921c5ae9e7ce925a2 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 0eaac80edcb4434d5f8953a44050720c5770faf3 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 8b88c9a2dbbb471d15e177f3d0a129256a19f228 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 57b7f888d1eb9e10447fe3190579aec6e05b661c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
 84574f0fca4596d475aed48797a2d4d6fad02915 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
 e771ce7de5a6849f48a036b2b1c30b435d8cb520 


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

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


Testing
---

Updated the unit test to test for bigger HMS snapshots


Thanks,

Alexander Kolbasov

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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




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

The previous code was serialized, this one isn't. It is most likely Ok 
because of the way clients are used now, but please 

1) Comment this fact
2) Double-check that we do not have concurrent access to the client.



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

there *is* connection problem



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

"Thrift call failed"



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

How do we know that this is SentryHdfsServiceException?



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

can we reuse the message for log and exception?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 32 (patched)
<https://reviews.apache.org/r/57901/#comment246208>

This comment doesn't explain what connect() is doing and is talking about 
implementation. Please update to describe at high level what connect() is 
supposed to provide without referring to specific implementation.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 39 (patched)
<https://reviews.apache.org/r/57901/#comment246209>

We do not want to add metrics on the interface, this comment probably 
should be fo rthe implementation parts.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 82 (original), 74 (patched)
<https://reviews.apache.org/r/57901/#comment246082>

The factory has the correct config, why create it multiple times?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 83 (patched)
<https://reviews.apache.org/r/57901/#comment246083>

The factory already has the correct config, why create a new one?



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

You need to pass SentryPolicyClientTransportConfig() to 
SentryPolicyServiceClientDefaultImpl() as well


- Alexander Kolbasov


On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -------
> 
> (Updated April 26, 2017, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> --

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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



- Alexander Kolbasov


On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 26, 2017, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 9d18063 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  6cea596 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  636de40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  12175f7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  038bca7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyStoreConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml e828d5e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java
>  ab12bf4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  28b1224 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
>  2a18b15 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java
>  4dc99a2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java
>  307d8c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
>  919b81b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java
>  d320d0f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
>  075983e 
>   
> sentry-

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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




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

The previous code was serialized, this one isn't. It is most likely Ok 
because of the way clients are used now, but please 

1) Comment this fact
2) Double-check that we do not have concurrent access to the client.



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

there *is* connection problem



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

"Thrift call failed"



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

How do we know that this is SentryHdfsServiceException?



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

can we reuse the message for log and exception?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 32 (patched)
<https://reviews.apache.org/r/57901/#comment246208>

This comment doesn't explain what connect() is doing and is talking about 
implementation. Please update to describe at high level what connect() is 
supposed to provide without referring to specific implementation.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 39 (patched)
<https://reviews.apache.org/r/57901/#comment246209>

We do not want to add metrics on the interface, this comment probably 
should be fo rthe implementation parts.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 82 (original), 74 (patched)
<https://reviews.apache.org/r/57901/#comment246082>

The factory has the correct config, why create it multiple times?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 83 (patched)
<https://reviews.apache.org/r/57901/#comment246083>

The factory already has the correct config, why create a new one?



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

You need to pass SentryPolicyClientTransportConfig() to 
SentryPolicyServiceClientDefaultImpl() as well


- Alexander Kolbasov


On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -------
> 
> (Updated April 26, 2017, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> --

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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




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

The previous code was serialized, this one isn't. It is most likely Ok 
because of the way clients are used now, but please 

1) Comment this fact
2) Double-check that we do not have concurrent access to the client.



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

there *is* connection problem



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

"Thrift call failed"



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

How do we know that this is SentryHdfsServiceException?



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

can we reuse the message for log and exception?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 32 (patched)
<https://reviews.apache.org/r/57901/#comment246208>

This comment doesn't explain what connect() is doing and is talking about 
implementation. Please update to describe at high level what connect() is 
supposed to provide without referring to specific implementation.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 39 (patched)
<https://reviews.apache.org/r/57901/#comment246209>

We do not want to add metrics on the interface, this comment probably 
should be fo rthe implementation parts.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 82 (original), 74 (patched)
<https://reviews.apache.org/r/57901/#comment246082>

The factory has the correct config, why create it multiple times?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 83 (patched)
<https://reviews.apache.org/r/57901/#comment246083>

The factory already has the correct config, why create a new one?



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

You need to pass SentryPolicyClientTransportConfig() to 
SentryPolicyServiceClientDefaultImpl() as well


- Alexander Kolbasov


On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -------
> 
> (Updated April 26, 2017, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> --

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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




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

The previous code was serialized, this one isn't. It is most likely Ok 
because of the way clients are used now, but please 

1) Comment this fact
2) Double-check that we do not have concurrent access to the client.



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

there *is* connection problem



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

"Thrift call failed"



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

How do we know that this is SentryHdfsServiceException?



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

can we reuse the message for log and exception?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 32 (patched)
<https://reviews.apache.org/r/57901/#comment246208>

This comment doesn't explain what connect() is doing and is talking about 
implementation. Please update to describe at high level what connect() is 
supposed to provide without referring to specific implementation.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
Lines 39 (patched)
<https://reviews.apache.org/r/57901/#comment246209>

We do not want to add metrics on the interface, this comment probably 
should be fo rthe implementation parts.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 82 (original), 74 (patched)
<https://reviews.apache.org/r/57901/#comment246082>

The factory has the correct config, why create it multiple times?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 83 (patched)
<https://reviews.apache.org/r/57901/#comment246083>

The factory already has the correct config, why create a new one?



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

You need to pass SentryPolicyClientTransportConfig() to 
SentryPolicyServiceClientDefaultImpl() as well


- Alexander Kolbasov


On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -------
> 
> (Updated April 26, 2017, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> --

Review Request 58782: SENTRY-1725 Include response status in TSentrySyncIDResponse

[Alexander Kolbasov]

/db/service/thrift/TSentryImportMappingDataRequest.java
 c8295f41d684d12714b316c7614d4cc8bb0b8784 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryImportMappingDataResponse.java
 1d38202b4b65dd4b7af8366213eef91a8f9d5e95 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryMappingData.java
 2c091cb729d4d2f069f3933739b7c407034b7f5b 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java
 15b339fd6fde5e146ebd37d5ce0ddf712e232924 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilegeMap.java
 9991f3b3448a6c066e8bcb79673a678f3b199c16 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryRole.java
 e60ac242d037e6928b826a26eff52afbbabdcba9 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentrySyncIDRequest.java
 53296d3743383eb811fb3a42d6b860671c31ee95 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentrySyncIDResponse.java
 374f21e5357e3563e0c299075b23fd9be01eefa9 
  
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/service/thrift/TSentryResponseStatus.java
 d9a9f86251b0b219014165a9be87b2a24b11703c 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 fb73783cb474a42e1d2c1da36af20d47b357d38f 
  
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
 b5c3390e2ea87a08e329f9dcc6bf587cd0e9cfc6 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Question about on-demand Sentry build

[Alexander Kolbasov]

Thanks Colm!

Would it be difficult to also parameterize it by the branch name?

- Alex

> On Apr 27, 2017, at 1:29 AM, Colm O hEigeartaigh  wrote:
> 
> Hi Alex,
> 
> Yes, I have updated it to the correct git repo. Let me know if there are
> still any problems with the builds.
> 
> Colm.
> 
> On Tue, Apr 25, 2017 at 6:35 AM, Alexander Kolbasov 
> wrote:
> 
>> Hello,
>> 
>> I discovered the build-on-demand page for Jenkins build:
>> 
>> https://builds.apache.org/view/PreCommit%20Builds/job/
>> PreCommit-SENTRY-Build-On-Demand/
>> 
>> When I tried it I discovered that it looks at the old version of Sentry at
>> https://git-wip-us.apache.org/repos/asf/incubator-sentry.git.
>> 
>> I think it would be useful to resurrect this job - does anyone know how to
>> update the configuration?
>> 
>> Thanks,
>> 
>> - Alex
>> 
> 
> 
> 
> -- 
> Colm O hEigeartaigh
> 
> Talend Community Coder
> http://coders.talend.com

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

> On April 26, 2017, 11:09 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
> > Lines 97 (patched)
> > <https://reviews.apache.org/r/57901/diff/6/?file=1700095#file1700095line97>
> >
> > The previous code was serialized, this one isn't. It is most likely Ok 
> > because of the way clients are used now, but please 
> > 
> > 1) Comment this fact
> > 2) Double-check that we do not have concurrent access to the client.

This isn't an issue, the top-level code is synchronized.


- Alexander


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


On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 26, 2017, 12:30 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 9d18063 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  6cea596 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  636de40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  12175f7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  038bca7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyStoreConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml e828d5e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java
>  ab12bf4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  28b1224 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
>  2a18b15 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java
>

Re: Review Request 58808: SENTRY-1726 sql changes to store last notification-id processed

[Alexander Kolbasov]

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



It is difficult to review these changes since there is no context in the JIRA, 
explaining how is this supposed to work. Here were just have some tables 
defined.

- Alexander Kolbasov


On April 27, 2017, 6:19 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58808/
> ---
> 
> (Updated April 27, 2017, 6:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1726
> https://issues.apache.org/jira/browse/SENTRY-1726
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1. Created new table to store the last notification id
> 2. Table is defined to store only one entry.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql
>  ba70715 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql
>  879e732 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql
>  e83ab83 
>   
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql
>  c28099b 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 
> 841dcaa 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 
> 223835e 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 
> 006d57b 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 
> 20921ea 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql
>  489ad66 
>   
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql
>  ec50912 
> 
> 
> Diff: https://reviews.apache.org/r/58808/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 58825: SENTRY-1695: Waiting for HMS notifications from Thrift should be interruptible

[Alexander Kolbasov]

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

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


Repository: sentry


Description
---

SENTRY-1695: Waiting for HMS notifications from Thrift should be interruptible


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 456dabc234031c00455a564aa2bd6ea87690ceb1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
 f593bffc827863955aa2591b3e387b2c7884242b 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java
 a70017857b5e48071a9653a211da0fa6d9c51c1e 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 57901: SENTRY-1593 Implement client failover for all the sentry clients

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 27, 2017, 7:42 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> ---
> 
> (Updated April 27, 2017, 7:42 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee 
> Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1593
> https://issues.apache.org/jira/browse/SENTRY-1593
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> At a high level the change set submitted does following.
> 1. Extends the capability of sentry name-node client and generic policy 
> client to connect to multiple servers and failover to the server that is 
> available
> 2. Transport handling of the clients is abstracted to another class so that 
> the client's just have to extend the class that abstracts it. Implementations 
> of the clients don't have to worry about it.
> 3. Changed the behavior of the clients so that they connect to one of the 
> configured servers when the client is actually instantiated.
> 
> As part of the code changes there are couple of files that are moved from one 
> package to another. Here is the list of them
> 1. RetryClientInvocationHandler.java
> 2. SentryClientInvocationHandler.java
> 3. ThriftUtil.java
> 4. SentryHdfsServiceException.java
> Amount these files, RetryClientInvocationHandler.java has changed. Please 
> have a closer look at this file.
> 
> 
> Diffs
> -
> 
>   sentry-core/sentry-core-common/pom.xml 9d18063 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  6cea596 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  636de40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  12175f7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  038bca7 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyStoreConstants.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  23552c2 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml e828d5e 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java
>  ab12bf4 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
>  28b1224 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java
>  2a18b15 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java
>  4dc99a2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java
>  307d8c3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
>  919b81b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java
>  d320d0f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
>  075983e 

Re: Review Request 58816: SENTRY-1721 Unit test failures in TestSentryStore due to wrong perm change count

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On April 27, 2017, 9:05 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58816/
> ---
> 
> (Updated April 27, 2017, 9:05 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix count at test
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d12b0da 
> 
> 
> Diff: https://reviews.apache.org/r/58816/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

[Alexander Kolbasov]

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




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

Do we need to worry about concurrent access? Can some other thread modify 
it while we read?


- Alexander Kolbasov


On April 27, 2017, 7:33 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> ---
> 
> (Updated April 27, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Check if credential provider already has the credential before creating a 
> credential
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  31a309b 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: SQL changes for SENTRY-1726

[Alexander Kolbasov]

Hello Kalyan,

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

I think it is better to store new notification ID as a new row .This will avoid 
holding a row lock for read-edify-write operation. The dwnside that we need to 
use MAX(), but given that tis is the primary key, MAX() should be an index scan 
rather then a table scan. And yes, we do need to prune old rows, we should be 
able to do that in the background to keep the table size small.

> 
> 
> 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 58811: SENTRY-1400 [Flaky test] TestSentryWebServerWithSSL times out

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
Line 187 (original), 187 (patched)
<https://reviews.apache.org/r/58811/#comment246327>

Can you clarify how does this change fixes the problem?


- Alexander Kolbasov


On April 27, 2017, 7:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58811/
> ---
> 
> (Updated April 27, 2017, 7:19 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add log message to find out if the key store path put in configuration is the 
> same as the one used by web server
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
>  dfd79ae 
> 
> 
> Diff: https://reviews.apache.org/r/58811/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58812: SENTRY-1718 TestSentryStore often fails in setup()

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


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

Please add a comment explaining the problem


- Alexander Kolbasov


On April 27, 2017, 7:33 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58812/
> ---
> 
> (Updated April 27, 2017, 7:33 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Check if credential provider already has the credential before creating a 
> credential
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  31a309b 
> 
> 
> Diff: https://reviews.apache.org/r/58812/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: SQL changes for SENTRY-1726

[Alexander Kolbasov]

This makes sense. Since normally HMSFollower is only running on one server, 
there shouldn’t be any contention, so read/modify/write should be fine in this 
case.

> On Apr 28, 2017, at 8:30 AM, Kalyan Kumar Kalvagadda  
> wrote:
> 
> Help me understand why should we be concerned about locking.
> 
> As far I know there are two threads that will be interested in last
> notification id
> 1. HMS Follower to construct  request to pull the latest notification
> 2. Sentry Metrics
> 
> HMSFollower should get this information from database every time it needs
> and have a in-memory copy. Other components like Metrics could depend on
> the in-memory copy stored by HMSFollower.
> Even if, we want to avoid using in-memory copy and use the database always,
> I don't know if locking would be an issue as there will not many requests
> to read the notification-id other then HMSFollower thread.
> 
> 
> -Kalyan
> 
> On Fri, Apr 28, 2017 at 2:00 AM, Alexander Kolbasov 
> wrote:
> 
>> Hello Kalyan,
>> 
>>> On Apr 27, 2017, at 9:14 PM, Kalyan Kumar Kalvagadda <
>> kkal...@cloudera.com> 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.
>> 
>> I think it is better to store new notification ID as a new row .This will
>> avoid holding a row lock for read-edify-write operation. The dwnside that
>> we need to use MAX(), but given that tis is the primary key, MAX() should
>> be an index scan rather then a table scan. And yes, we do need to prune old
>> rows, we should be able to do that in the background to keep the table size
>> small.
>> 
>>> 
>>> 
>>> 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 58854: Add log message for key store path in test file SentryServiceIntegrationBase

[Alexander Kolbasov]

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



What JIRA is this?

- Alexander Kolbasov


On April 28, 2017, 8:22 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58854/
> ---
> 
> (Updated April 28, 2017, 8:22 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add debug log message to help find root cause of SENTRY-1400
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
>  dfd79ae 
> 
> 
> Diff: https://reviews.apache.org/r/58854/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58854: Add log message for key store path in test file SentryServiceIntegrationBase

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
Lines 189 (patched)
<https://reviews.apache.org/r/58854/#comment246396>

Do we support this style of format string? In other places we use {}-style 
formatting.


- Alexander Kolbasov


On April 28, 2017, 8:22 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58854/
> ---
> 
> (Updated April 28, 2017, 8:22 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add debug log message to help find root cause of SENTRY-1400
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
>  dfd79ae 
> 
> 
> Diff: https://reviews.apache.org/r/58854/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58799: SENTRY-1730 - Remove FileInputStream/FileOutputStream

[Alexander Kolbasov]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
Line 45 (original), 44 (patched)
<https://reviews.apache.org/r/58799/#comment246399>

Do you need to close the output stream?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
Line 67 (original), 66 (patched)
<https://reviews.apache.org/r/58799/#comment246400>

THis would cause a double-close of input


- Alexander Kolbasov


On April 27, 2017, 2:50 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58799/
> ---
> 
> (Updated April 27, 2017, 2:50 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1730
> https://issues.apache.org/jira/browse/SENTRY-1730
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This task is to remove FileInputStream/FileOutputStream due to reasons 
> outlined here:
> 
> https://www.cloudbees.com/blog/fileinputstream-fileoutputstream-considered-harmful
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  c1518ba3 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
>  4a632bc9 
> 
> 
> Diff: https://reviews.apache.org/r/58799/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Review Request 58867: SENTRY-1734 Create/Alter/Drop database/table should check corresponding property before drop priv...

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1734 Create/Alter/Drop database/table should check corresponding 
property before drop priv...


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 ca4487f279f435c45be9ded7bd00086f2bbd9b02 


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


Testing
---


Thanks,

Alexander Kolbasov

Question about DelegateSentryStore class

[Alexander Kolbasov]

It looks like we have a very "generic" code in SentryGenericPolicyProcessor
that creates DelegateSentryStore based on the class name which may be
changed in configuration. This is very cute, but sounds quite complicated
to me. I don't know the history here, but I would suggest removing this and
just creating DelegateSentryStore directly.

Any thoughts/comments about this?

- Alex

Re: Review Request 58883: SENTRY-1742: Upgrade to Maven surefire plugin v2.2

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 1, 2017, 1:55 p.m., Jan Hentschel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58883/
> ---
> 
> (Updated May 1, 2017, 1:55 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1742
> https://issues.apache.org/jira/browse/SENTRY-1742
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Upgraded the **maven-surefire-plugin** to version 2.20.
> 
> 
> Diffs
> -
> 
>   pom.xml 66e9220 
> 
> 
> Diff: https://reviews.apache.org/r/58883/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jan Hentschel
> 
>

Re: Review Request 58854: SENTRY-1733 Add log message for key store path in test file SentryServiceIntegrationBase

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 1, 2017, 3:02 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58854/
> ---
> 
> (Updated May 1, 2017, 3:02 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> add debug log message to help find root cause of SENTRY-1400
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
>  dfd79ae 
> 
> 
> Diff: https://reviews.apache.org/r/58854/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 58896: SENTRY-1744 Simplify creation of DelegateSentryStore

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1744 Simplify creation of DelegateSentryStore


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
 3be6a0bf38d03f81df4aae3a878d1e6ecae52be7 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
 b638be7456b4bbe27b48691b52912f06d43e09f1 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/PolicyStoreConstants.java
 8cf1c1a16a26939f4cf68278c7b8204ec0b08ca7 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java
 de663138e63ccb998ec94ac42475071770a3526c 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 58913: SENTRY-1745: Bundle sentry-core-common into sentry-hdfs-dist to avoid NN failing with NoClassDefFoundError

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 2, 2017, 3:39 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58913/
> ---
> 
> (Updated May 2, 2017, 3:39 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> With the recent refactoring change SENTRY-1639, we introduced a new class 
> SentryClientTransportConfigInterface. While trying to spin up a real cluster 
> with this change, NN could fail to start with NoClassDefFoundError as we only 
> put the jars under SENTRY_HOME/lib/plugins on the HDFS classpath and this new 
> class is part of sentry-core-common which is not bundled inside plugins/.
> 
> 
> Diffs
> -
> 
>   sentry-hdfs/sentry-hdfs-dist/pom.xml dd2d847 
> 
> 
> Diff: https://reviews.apache.org/r/58913/diff/1/
> 
> 
> Testing
> ---
> 
> Verified to see the sentry-core-common being bundled.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Review Request 58926: SENTRY-1747: HMSFollower shouldn't create local hive during tests

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1747: HMSFollower shouldn't create local hive during tests


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 ec8676e5c4e4030b4a8d819bec028694b96fd189 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 58825: SENTRY-1695: Waiting for HMS notifications from Thrift should be interruptible

[Alexander Kolbasov]

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

(Updated May 3, 2017, 7:01 p.m.)


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


Changes
---

Addressed code review comments from Sergio


Repository: sentry


Description
---

SENTRY-1695: Waiting for HMS notifications from Thrift should be interruptible


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
 456dabc234031c00455a564aa2bd6ea87690ceb1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
 f593bffc827863955aa2591b3e387b2c7884242b 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java
 a70017857b5e48071a9653a211da0fa6d9c51c1e 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Re: Sentry HA redesign and Viewfs support

[Alexander Kolbasov]

Hello Qianbo,

The SENTRY-1627 doesn’t contain any details of the suggested integration 
between Sentry and Viewfs. Can you explain in more details what changes do you 
make in the Sentry code to support it?

Sentry HA changes do affect some of the HDFS sync code path, but it is 
difficult to tell whether this is an issue for Viewfs or not.

Can you check the sentry-ha-redesign branch and see whether there are any red 
flags for you?

Thanks,

- Alex Kolbasov


> On May 3, 2017, at 12:00 PM, Qianbo Huai  wrote:
> 
> Hi Sentry Dev,
> 
> A couple of month ago, we created a viewfs feature request (SENTRY-1627).
> Viewfs
> 
> is critical in supporting the rapid growth / expansion of our HDFS
> clusters. Since then, we have implemented viewfs support on top of Sentry
> cdh5-1.5.1_5.7.2. The implementation is currently in the process of being
> rolled out to our production. At the high level, the change involves
> patching the Sentry service thrift API so that NN plugin of each HDFS
> clusters can identify itself to the service. In turn, the sentry service
> converts viewfs paths to HDFS paths that the requesting NN plugin can
> understand.
> 
> As you are redesigning HA for sentry and moving in-memory states to
> database, it would be great that you support viewfs to begin with, or be
> aware of the need for viewfs support at a later stage.
> 
> Thanks
> Qianbo Huai
> 
> Uber Data Infra Engineering

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

y/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
Line 421 (original), 424 (patched)
<https://reviews.apache.org/r/58221/#comment246914>

Can you explain this change?



sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
Line 441 (original), 444 (patched)
<https://reviews.apache.org/r/58221/#comment246915>

Please use string constant for col1_role instead of copying it in multiple 
places.


- Alexander Kolbasov


On May 3, 2017, 7:20 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated May 3, 2017, 7:20 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  ec8676e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  ce73358 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  834ed41 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/23/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 171 (original), 171 (patched)
<https://reviews.apache.org/r/58221/#comment246933>

It looks like we can move it back to the start service since you fixed the 
problem with HMSFollower.


- Alexander Kolbasov


On May 3, 2017, 7:20 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated May 3, 2017, 7:20 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  ec8676e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  ce73358 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  834ed41 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/23/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On May 4, 2017, 5:47 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
> > Lines 322 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707217#file1707217line352>
> >
> > it would be easier to read the code by keeping this section outside of 
> > previous finally block

This isn't about being easier to read. What may happen is that you get 
exception and the rest of the code isn't executed. The question is what code 
*must* be executed.


> On May 4, 2017, 5:47 a.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
> > Lines 165 (patched)
> > <https://reviews.apache.org/r/58221/diff/23/?file=1707218#file1707218line165>
> >
> > Other functions in this class are public. Why do we want to limit it to 
> > just package private?

Usually we do not want to make anything public unless we need to. This 
particular function is new and there is no need for it to be public.


- Alexander


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


On May 4, 2017, 5:48 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated May 4, 2017, 5:48 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  ec8676e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  ce73358 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  834ed41 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/24/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58976: SENTRY-1709 Avoid randomizing the servers at client side based on configuration.

[Alexander Kolbasov]

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




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 112 (patched)
<https://reviews.apache.org/r/58976/#comment247055>

I would suggest a better name for it. This setting, in effect, controls not 
the randomization but the load balancing between servers.

So I'd suggest the name 

isLoadBalancingEnabled()

and the corresponding config be named

sentry.service.client.connection.loadbalance



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 103 (patched)
<https://reviews.apache.org/r/58976/#comment247056>

Naming - I would suggest the name
SENTRY_CLIENT_LOAD_BALANCING



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 104 (patched)
<https://reviews.apache.org/r/58976/#comment247057>

Naming - I would suggest 

sentry.service.client.connection.loadbalance



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Lines 150 (patched)
<https://reviews.apache.org/r/58976/#comment247058>

We also want to check that the endpoints list has at least two elements.


- Alexander Kolbasov


On May 3, 2017, 9:29 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58976/
> ---
> 
> (Updated May 3, 2017, 9:29 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1709
> https://issues.apache.org/jira/browse/SENTRY-1709
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Clients should randomize the server list based on the confguration.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  24192fd 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  4af7d1f 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  64cdd46 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  396a7f6 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  9ddb400 
> 
> 
> Diff: https://reviews.apache.org/r/58976/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59006: SENTRY-1749: Sentry fails to setup secure connection to HMS if the service unix user is missing active tgt

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 4, 2017, 9:58 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59006/
> ---
> 
> (Updated May 4, 2017, 9:58 p.m.)
> 
> 
> Review request for sentry and Alexander Kolbasov.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Looking at the code of Sentry and HiveMetaStoreClient, it looks like this 
> problem has been there for a long time. But it's mostly masked by the 
> restarts we are doing after the clean deployment.
> 
> So during the course of initial service start, sentry and other services 
> don't have any keytabs active under local unix user and during this time, the 
> log of sentry is cluttered with the above errors. But after we do a restart 
> after deployment completes, the Sentry code picks up the principal that was 
> activated under "sentry" user (which users do it for testing purposes) and 
> thus works properly.
> 
> *Ideally* the service processes shouldn't depend on the keytabs active on 
> running unix user but rather use the keytabs supplied by startup system in 
> the process directory.
> 
> Looking at the code of 
> [HMSFollower|https://github.com/apache/sentry/blob/sentry-ha-redesign/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java]
>  we invoke the classes of Hive - HiveMetaStoreClient which exclusively uses 
> the UserGroupInformation object. And for UserGroupInformation object to pick 
> up the keytab, one should explicitly call the methods like 
> [loginUserFromKeytabAndReturnUGI|https://github.com/apache/hadoop/blob/61858a5c378da75aff9cde84d418af46d718d08b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L1406]
>  or use 
> [getUGIFromSubject|https://github.com/apache/hadoop/blob/61858a5c378da75aff9cde84d418af46d718d08b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L841].
>  For the fact that HMSFollower already logs into the keytab, we can leverage 
> the method getUGIFromSubject() to make the UserGroupInformation aware of 
> keytab
  authentication.
> 
> If UGI object is not made aware of keytab, then the invocation of 
> [UserGroupInformation#getCurrentUser|https://github.com/apache/hadoop/blob/61858a5c378da75aff9cde84d418af46d718d08b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L736-L742]
>  falls back to use Unix user.
> 
> To avoid this, we need to add extra logic to HMSFollower to make 
> UserGroupInformation aware of the keytab that way it uses it for 
> communication with HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  ec8676e 
> 
> 
> Diff: https://reviews.apache.org/r/59006/diff/1/
> 
> 
> Testing
> ---
> 
> Successfully verified that without keytab active on the unix user, the 
> program was successfully able to connect to HMS using the supplied keytab.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

Re: Review Request 58976: SENTRY-1709 Avoid randomizing the servers at client side based on configuration.

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 4, 2017, 11:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58976/
> ---
> 
> (Updated May 4, 2017, 11:24 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, 
> Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1709
> https://issues.apache.org/jira/browse/SENTRY-1709
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Clients should randomize the server list based on the confguration.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
>  24192fd 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  4af7d1f 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
>  64cdd46 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
>  396a7f6 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
>  9ddb400 
> 
> 
> Diff: https://reviews.apache.org/r/58976/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Line 172 (original), 172 (patched)
> > <https://reviews.apache.org/r/58221/diff/24/?file=1708101#file1708101line172>
> >
> > Understand that this delay is causing issues during tests but making it 
> > 0 may not be a best choice.
> > 
> > It may take some time for the server to initialize the database and be 
> > completely up to handle the notifications. 
> > 
> > If 6 milli sec is higher value, we should have some smaller value 
> > instead.
> 
> Na Li wrote:
> since we prevent connecting to local meta store, HMSFollower does nothing 
> if Hive meta store is not up. It does not hurt to have this value being zero, 
> and allow sentry getting updates sooner.
> 
> Na Li wrote:
> "sentry-1753 Make HMSFollower initial delay configurable" is created

The initial delay was coming out of the blue and isn't clear why it is needed 
at all. What kind of server initialization are you talking about?


- Alexander


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


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> -------
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  99549bc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  ce73358 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  834ed41 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java
>  2ebe561 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
>  212c465 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  b5247d0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 58221: SENTRY-1649 move HMS follower to runServer

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On May 5, 2017, 3:47 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58221/
> ---
> 
> (Updated May 5, 2017, 3:47 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar 
> kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1649 move HMS follower to runServer
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  99549bc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  132db63 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  ce73358 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  834ed41 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java
>  2ebe561 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
>  212c465 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  b5247d0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  1ace07c 
> 
> 
> Diff: https://reviews.apache.org/r/58221/diff/25/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Na Li
> 
>