Re: Review Request 60926: SENTRY-1760: HMSFollower should detect when a full snapshot from HMS is required

[Alexander Kolbasov]

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




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

Can you achieve the same purpose by providing client in a constructor? This 
may be a bit cleaner.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 149 (original), 156 (patched)
<https://reviews.apache.org/r/60926/#comment256304>

It would be very good to have a complete description of the algorithm used 
to decide whether a full snapshot is needed - it may be better to put it as a 
block comment at the top of the file or even in the class javadoc.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 151 (original), 158 (patched)
<https://reviews.apache.org/r/60926/#comment256305>

Do we always have at least one object? Can Hive have a valid empty path 
list or it is not possible?

The second part about the gaps - please provide more ddetail - gaps between 
what and what?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 152 (original), 159 (patched)
<https://reviews.apache.org/r/60926/#comment256306>

What if we have an old snapshot so it isn't empty (and in fact we keep some 
number of old snapshots around) so this will only be the case the first time. 
How is the decision made afterwords?



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

Why do we need to keep in in the class field?

This is rather weird - we detect that we need a full snapshot, and instead 
of just getting it, record this piece of information and retrieve it during the 
next cycle. What is the point in doing it this way?



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

Suppose that we have two Sentry servers. The first one got the current 
notification ID, but later looses leadership and another one processes a few 
requests by the time we get here, so we can run into this situation. So we 
should be a bit more conservative and may be check a few things first:

1) Check that we are still the leader
2) Re-read currentNotificationId and make sure it didn't advanced forward.

Once we detect the condition we should just get the snapshot rather then 
defer it to the next iteration.



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

Does HMS provide a guarantee that notifications have no gaps?



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

Please document this function.



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

This is a strange way of doing it. Why can't we check the first 
notification instead? Why are we checking the size instead? What is it that you 
are actually checking?

What if HMS notifications contain duplicate IDs (and we know that this is 
possible) - this will mess up the logic.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Line 217 (original)
<https://reviews.apache.org/r/60926/#comment256313>

Is this no longer relevant or the logic is moved elsewhere?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 239 (patched)
<https://reviews.apache.org/r/60926/#comment256314>

can eventId be null here?


- Alexander Kolbasov


On July 18, 2017, 8:23 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60926/
> -------
> 
> (Updated July 18, 2017, 8:23 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: sentry-1760
> https://issues.apache.org/jira/browse/sentry-1760
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch will set the

Re: Review Request 60926: SENTRY-1760: HMSFollower should detect when a full snapshot from HMS is required

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On July 19, 2017, 8:13 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60926/
> ---
> 
> (Updated July 19, 2017, 8:13 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: sentry-1760
> https://issues.apache.org/jira/browse/sentry-1760
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> The patch will set the 'requestHmsSnapshot' to TRUE whenever the following 
> cases are found:
> 
> * List of notifications received are different than expected. 
>   This may happen when Sentry has been down or HDFS sync was disabled for a 
> while (more than 24h), 
>   and the HMS cleared old notifications (older than 24h) not processed by 
> Sentry causing a gap when retrieving notifications.
> * Latest Sentry notification ID processed is bigger than current HMS 
> notification ID.
>   This may happen when the HMS DB data was reset or restore from an old 
> snapshot causing sync issues with Sentry.
> 
> New snapshots are persisted with a new generation ID (or image number), so 
> there's is no need to clean-up older snapshots.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  083ee4c247f96d5c87b44b9785663a2783e6644d 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  05518e81f52965dc1ff102dcdd446010381b9a7a 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  d67c16258c67aae997de4c0451c8b642ab05d298 
> 
> 
> Diff: https://reviews.apache.org/r/60926/diff/5/
> 
> 
> Testing
> ---
> 
> Added test cases on TestHmsFollower
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table doesn't cleanup the permissions associated with it

[Alexander Kolbasov]

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




sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
Lines 90 (patched)
<https://reviews.apache.org/r/60883/#comment256536>

This file has only cosmetic changes - do we really need these changes? Less 
files to touch.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java
Line 54 (original), 54 (patched)
<https://reviews.apache.org/r/60883/#comment256537>

Why this change? The original comment was reasonable and matches the code.



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

This field isn't really used for anything, just for tests which can easily 
extract the value from NotificationProcessor.



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

Should it really be public?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 304 (original), 312 (patched)
<https://reviews.apache.org/r/60883/#comment256540>

Why is this public?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
Line 59 (original), 59 (patched)
<https://reviews.apache.org/r/60883/#comment256538>

Why this change? Ommitting public works fine.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
Lines 19 (patched)
<https://reviews.apache.org/r/60883/#comment256542>

Why this test lives here rather then in the same package as HMSFollower?



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 213 (patched)
<https://reviews.apache.org/r/60883/#comment256543>

Do you need @Test here?

Please document what this test case is testing



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 223 (patched)
<https://reviews.apache.org/r/60883/#comment256544>

Please document what this case is testing. Same for tests below.


- Alexander Kolbasov


On July 21, 2017, 2:30 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> ---
> 
> (Updated July 21, 2017, 2:30 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not 
> from hive-site.xml through hiveConf. The input configuration for HMSFollower 
> constructor is from sentry-site.xm. We should use that configuration to get 
> server name.
> 2) There are two variable names that could hold the value of the server. 
> "hive.sentry.server"is deprecated and is in 
> HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We 
> should check the sentry.hive.server first. If it is not set, check the 
> deprecated hive.sentry.server to be backward compatible. If it is still not 
> set, use default value
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  e1312bf 
>   
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java
>  dccbbb6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  547a61f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  3d67401 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  fdf52bf 
> 
> 
> Diff: https://reviews.apache.org/r/60883/diff/5/
> 
> 
> Testing
> ---
> 
> create integration tests. reproduced the issue and verfied the fix
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: SENTRY-1855: PERM/PATH transactions can fail to commit to the sentry database under load

[Alexander Kolbasov]

Lina, Thank you for sending a detailed summary!


> On Jul 22, 2017, at 12:29 AM, Na Li  > wrote:
> 
> Hello,
> 
> Can you provide your feedback on this issue? We need to make decision soon.
> 
> 1) Summary
> 
> The current approach updates changeID (primary key for permission change and 
> path change) manually. In a single transaction, the code reads the max 
> changeID from DB, increases it by one, and save the value in new change 
> entry. If two threads are adding changes into DB at the same time, collision 
> happens (primary key does not allow multiple entries having the same value), 
> and one transaction fails. Then the failed transaction goes through multiple 
> re-tries. If retry count reaches max value, the transaction fails.
> 
> In our stress testing on a single sentry server, with 15 clients doing 
> grant/revoke operations concurrently, we saw multiple transaction failures, 
> and the exponential-back-off retry increases the latency of every transaction 
> in sentry. We have serious performance issue on saving permission and path 
> updates.

How important is the performance issue? Do we expect high rate of permission 
updates in realistic scenarios? Do we have any idea about current supported 
rate of permission updates?

Note that in our single sentry server case the code that updates the DB is 
serialized, and, thus, we shouldn’t see any collisions at the DB level at all. 
This means that there is some other bug hiding here, still undiscovered. The 
retries and exponential backoff shouldn’t apply in the single-server case at 
all.

> 
> 2) Potential solutions
> 
> 2.1) Find out why we have collision on a single sentry server with 
> synchronization on saving updates. Once find the cause, fix it.

+1 on that

> + Follow existing approach. Does not introduce big change to the code base. 
> - Need time to investigate why synchronization at application level on a 
> single sentry server does not prevent key collision.
> - Does not scale. All updates are serialized, not much concurrency. 
> - Still have key collision exception and transaction failure when more than 
> one Sentry servers are deployed. 
> - Transaction failure at collision increases time to execute a transaction. 
> - It is confusing to customer that there are transaction failure in normal 
> operation. Increase support cases
> 
> 2.2) CDH-transactionFailure.01-cdh5-1.5.1.patch
> The patch that achieves 5 times or more performance increase than the current 
> approach.
> It contains the following changes
> revert sentry-1795 (so the changeID is auto-increment. This avoids key 
> collision. This is main reason we have performance improvement)
> revert sentry-1824 (no need to synchronize when changeID is auto-increment)
> get continuous delta list from SentryStore even when the delta list has hole 
> (for example, the list is 1,2,3,5,6, return 1,2,3. If the hole is at the 
> front of list, return full snapshot)

Please document the algorithm you are suggesting in more detail. How do you 
propose to handle synchronization between NN and Sentry server using 
auto-incremented IDs?

Performance is an important consideration once we achieve correctness. It would 
be great to have *correct* solution that also performs well.
> + Relative small changes. Verified working with good performance when there 
> is no transaction failure
> + When the hole in delta list is temporary (transaction in flight), return 
> the continuous delta list is effective to deal with the hole. Most likely, 
> the hole will disappear next time HDFS requests for changes.
> 
> - When there is transaction failure (the hole in changeID is permanent), 
> sends back full snapshot, which is expensive. If we can detect permanent 
> hole, then we don't need to send full snapshot, which is very expensive, and 
> may exhaust memory for big customer
> 

Right, sending full snapshot is extremely expensive. How do you propose to 
detect such permanent hole?
> 2.3) Use timestamp to sort the changes
> 
> a) use timestamp, such as MSentryPermChange.createTimeMs or 
> MSentryPathChange.createTimeMs to sort the entries. If there are more than 
> one entry having same timestamp, use changeID to break the tie.

IMO we should use either timestamps or changeIDs, not both.

> b) HDFS asks for updates using these timestamp values instead of changeID. 

I think that this approach may cause some updates to be lost.


> c) changeID is primary key, only used to uniquely identify the entry, and not 
> required to be sequential nor consecutive.
> d) Purge the change entries in DB using timestamp instead of changeID. For 
> example, keep 3 polling intervals of entries to allow HDFS getting the 
> changes before they are purged.

Suppose that you do this purging. Later you restart NN and it asks for all 
updates since time T, but a bunch of updates since this time were purged, so 
they never reach NN and are permanently lost. And it is impossible to detect 
that thi

Splitting permissions and path updates

[Alexander Kolbasov]

Lina’s email prompted an interesting thought - right now when the NameNode 
plugin isn’t happy and wants a full update we send it both permissions update 
and path update. Path update is very expensive while permissions update is 
usually much smaller.

It would be very useful to be able to send just full path update or just full 
perms update when there is a problem with just path or just permissions.

Any thoughts?

- Alex

Re: JIRA versions

[Alexander Kolbasov]

I am in the process of doing that

On Mon, Jul 24, 2017 at 4:41 PM, Mat Crocker 
wrote:

> Hi Colm,
>
> Can you confirm this is the set you are trying to migrate?
>
> "project = sentry and (fixVersion = sentry-ha-redesign or affectedVersion =
> sentry-ha-redesign) "
>
> Happy to help.
>
>
>
> On Mon, Jul 24, 2017 at 3:21 AM, Colm O hEigeartaigh 
> wrote:
>
> > Can someone with admin privileges on JIRA make this happen? Just bulk
> move
> > all sentry-ha-redesign issues to 2.0.0 + delete the sentry-ha-redesign
> > version once this is done.
> >
> > Colm.
> >
> > On Thu, Jul 20, 2017 at 4:26 PM, Kalyan Kumar Kalvagadda <
> > kkal...@cloudera.com> wrote:
> >
> > > Colm,
> > >
> > > I agree with you.
> > >
> > > -Kalyan
> > >
> > > On Thu, Jul 20, 2017 at 10:25 AM, Colm O hEigeartaigh <
> > cohei...@apache.org
> > > >
> > > wrote:
> > >
> > > > Hi all,
> > > >
> > > > Now that the ha branch is associated with the "2.0.0" release,
> perhaps
> > in
> > > > JIRA we could remove the "sentry-ha-redesign" version and move all of
> > the
> > > > existing issues to the "2.0.0" version? As things stand we have two
> > > > different versions that correspond to the same release.
> > > >
> > > > Colm.
> > > >
> > > >
> > > > --
> > > > Colm O hEigeartaigh
> > > >
> > > > Talend Community Coder
> > > > http://coders.talend.com
> > > >
> > >
> >
> >
> >
> > --
> > Colm O hEigeartaigh
> >
> > Talend Community Coder
> > http://coders.talend.com
> >
>
>
>
> --
> Mat Crocker
> Engineering Manager, Cloudera Inc.
>

Re: JIRA versions

[Alexander Kolbasov]

Done!

On Mon, Jul 24, 2017 at 10:21 AM, Colm O hEigeartaigh 
wrote:

> Can someone with admin privileges on JIRA make this happen? Just bulk move
> all sentry-ha-redesign issues to 2.0.0 + delete the sentry-ha-redesign
> version once this is done.
>
> Colm.
>
> On Thu, Jul 20, 2017 at 4:26 PM, Kalyan Kumar Kalvagadda <
> kkal...@cloudera.com> wrote:
>
> > Colm,
> >
> > I agree with you.
> >
> > -Kalyan
> >
> > On Thu, Jul 20, 2017 at 10:25 AM, Colm O hEigeartaigh <
> cohei...@apache.org
> > >
> > wrote:
> >
> > > Hi all,
> > >
> > > Now that the ha branch is associated with the "2.0.0" release, perhaps
> in
> > > JIRA we could remove the "sentry-ha-redesign" version and move all of
> the
> > > existing issues to the "2.0.0" version? As things stand we have two
> > > different versions that correspond to the same release.
> > >
> > > Colm.
> > >
> > >
> > > --
> > > Colm O hEigeartaigh
> > >
> > > Talend Community Coder
> > > http://coders.talend.com
> > >
> >
>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Sentry-jdk-1.7-v2 - Build # 100 - Still Failing

[Alexander Kolbasov]

Thanks Dapeng!

On Mon, Jul 24, 2017 at 10:12 AM, Sun, Dapeng  wrote:

> The failure is happened on ubuntu-eu2, I have removed this node from build
> list.
>
> -Original Message-
> From: Apache Jenkins Server [mailto:jenk...@builds.apache.org]
> Sent: Monday, July 24, 2017 4:06 PM
> To: s...@apache.org; ak...@cloudera.com; cohei...@apache.org;
> hao@cloudera.com; dev@sentry.apache.org
> Subject: Sentry-jdk-1.7-v2 - Build # 100 - Still Failing
>
> The Apache Jenkins build system has built Sentry-jdk-1.7-v2 (build #100)
>
> Status: Still Failing
>
> Check console output at https://builds.apache.org/job/
> Sentry-jdk-1.7-v2/100/ to view the results.
>

Re: JIRA versions

[Alexander Kolbasov]

Removed.

On Mon, Jul 24, 2017 at 5:02 PM, Colm O hEigeartaigh 
wrote:

> I still see the following version in JIRA:
>
> https://issues.apache.org/jira/projects/SENTRY/versions/12336080
>
> Colm.
>
> On Mon, Jul 24, 2017 at 3:57 PM, Alexander Kolbasov 
> wrote:
>
>> Done!
>>
>> On Mon, Jul 24, 2017 at 10:21 AM, Colm O hEigeartaigh <
>> cohei...@apache.org> wrote:
>>
>>> Can someone with admin privileges on JIRA make this happen? Just bulk
>>> move
>>> all sentry-ha-redesign issues to 2.0.0 + delete the sentry-ha-redesign
>>> version once this is done.
>>>
>>> Colm.
>>>
>>> On Thu, Jul 20, 2017 at 4:26 PM, Kalyan Kumar Kalvagadda <
>>> kkal...@cloudera.com> wrote:
>>>
>>> > Colm,
>>> >
>>> > I agree with you.
>>> >
>>> > -Kalyan
>>> >
>>> > On Thu, Jul 20, 2017 at 10:25 AM, Colm O hEigeartaigh <
>>> cohei...@apache.org
>>> > >
>>> > wrote:
>>> >
>>> > > Hi all,
>>> > >
>>> > > Now that the ha branch is associated with the "2.0.0" release,
>>> perhaps in
>>> > > JIRA we could remove the "sentry-ha-redesign" version and move all
>>> of the
>>> > > existing issues to the "2.0.0" version? As things stand we have two
>>> > > different versions that correspond to the same release.
>>> > >
>>> > > Colm.
>>> > >
>>> > >
>>> > > --
>>> > > Colm O hEigeartaigh
>>> > >
>>> > > Talend Community Coder
>>> > > http://coders.talend.com
>>> > >
>>> >
>>>
>>>
>>>
>>> --
>>> Colm O hEigeartaigh
>>>
>>> Talend Community Coder
>>> http://coders.talend.com
>>>
>>
>>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Splitting permissions and path updates

[Alexander Kolbasov]

Does it mean that without any changes the current code may send e.g. full
update for permissions and partial update for paths or visa versa?

- Alex

On Mon, Jul 24, 2017 at 5:36 PM, Na Li  wrote:

> Sasha,
>
> When NameNode plugin asks for updates, it includes info for both permission
> and path. However, the processing is separate. It is possible for Sentry to
> send full snapshot of permission and delta change to HDFS. At Sentry, perm
> and path processing share the same class, but they have their own
> instances.
>
> The current behavior can already satisfy your requirements. I have
> confirmed this with Sergio.
>
> You can see it in SentryPlugin.java at sentry server side.
>
>   public void initialize(Configuration conf, SentryStore sentryStore)
> throws SentryPluginException {
> PermImageRetriever permImageRetriever = new
> PermImageRetriever(sentryStore);
> PathImageRetriever pathImageRetriever = new
> PathImageRetriever(sentryStore);
> PermDeltaRetriever permDeltaRetriever = new
> PermDeltaRetriever(sentryStore);
> PathDeltaRetriever pathDeltaRetriever = new
> PathDeltaRetriever(sentryStore);
> pathsUpdater = new DBUpdateForwarder<>(pathImageRetriever,
> pathDeltaRetriever);   <- path has its own instance
> permsUpdater = new DBUpdateForwarder<>(permImageRetriever,
> permDeltaRetriever); <- perm has its own instance
> ...
> }
>
> Thanks,
>
> Lina
>
> On Fri, Jul 21, 2017 at 6:35 PM, Alexander Kolbasov 
> wrote:
>
> > Lina’s email prompted an interesting thought - right now when the
> NameNode
> > plugin isn’t happy and wants a full update we send it both permissions
> > update and path update. Path update is very expensive while permissions
> > update is usually much smaller.
> >
> > It would be very useful to be able to send just full path update or just
> > full perms update when there is a problem with just path or just
> > permissions.
> >
> > Any thoughts?
> >
> > - Alex
>

Re: Review Request 60955: SENTRY-1853: Add the log level access mechanism

[Alexander Kolbasov]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 1 (patched)
<https://reviews.apache.org/r/60955/#comment256698>

In all other files we have the copyright first, followed by the package 
description.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 29 (patched)
<https://reviews.apache.org/r/60955/#comment256706>

Please reformat this file using 2 spaces identation instead of 4 spaces.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 31 (patched)
<https://reviews.apache.org/r/60955/#comment256700>

this can be private



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 32 (patched)
<https://reviews.apache.org/r/60955/#comment256699>

Why is the size specified in the code? Is it the standard practice?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 38 (patched)
<https://reviews.apache.org/r/60955/#comment256701>

can be private
Also, please add javadoc.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 48 (patched)
<https://reviews.apache.org/r/60955/#comment256708>

Please add comment, explaining what this function does.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 50 (patched)
<https://reviews.apache.org/r/60955/#comment256707>

Where is logname used?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 57 (patched)
<https://reviews.apache.org/r/60955/#comment256703>

`` is used multiple times - would it make sense to create a 
constant for it?

Also, here and in other places you output the input as-is. This may create 
cross-scripting vulnerability. I think it should be url-escaped first before 
sending out.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 73 (patched)
<https://reviews.apache.org/r/60955/#comment256705>

This is last statement, so return should be ommitted



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
Lines 107 (patched)
<https://reviews.apache.org/r/60955/#comment256709>

Is it possible to move it under /admin ?



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
Lines 48 (patched)
<https://reviews.apache.org/r/60955/#comment256711>

Please add comment explaining what this (and the next) test case actually 
tests.



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
Lines 54 (patched)
<https://reviews.apache.org/r/60955/#comment256710>

Can we also check that the actual level is set in the logger?



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
Lines 63 (patched)
<https://reviews.apache.org/r/60955/#comment256712>

Should it also check that the log level matches whatever logger thinks it 
is?


- Alexander Kolbasov


On July 24, 2017, 7:39 a.m., Donghui Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60955/
> -------
> 
> (Updated July 24, 2017, 7:39 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: SENTRY-1853
> https://issues.apache.org/jira/browse/SENTRY-1853
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add the log level setting and reading mechanism, through which users can 
> control the log content.
> This mechanism provides the url interface,through which users can dynamically 
> set the log level. Service restores the original log level after restart.
> 
> The format of the interface is as follows:
> 1. Read the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME
> 2. Set the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME & level =

Re: Review Request 60955: SENTRY-1853: Add the log level access mechanism

[Alexander Kolbasov]

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




sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
Lines 37 (patched)
<https://reviews.apache.org/r/60955/#comment256713>

Why do we need an empty before/after methods? Can these be removed?


- Alexander Kolbasov


On July 24, 2017, 7:39 a.m., Donghui Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60955/
> ---
> 
> (Updated July 24, 2017, 7:39 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: SENTRY-1853
> https://issues.apache.org/jira/browse/SENTRY-1853
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add the log level setting and reading mechanism, through which users can 
> control the log content.
> This mechanism provides the url interface,through which users can dynamically 
> set the log level. Service restores the original log level after restart.
> 
> The format of the interface is as follows:
> 1. Read the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME
> 2. Set the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME & level = LOG_LEVEL
> 
> The above parameters include:
> HOST: host name or ip address of sentry server.
> PORT: port of sentry server.
> CLASS_NAME: name of class whose log level is read or set.
> LOG_LEVEL: the log level to be set.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
>  01f3a0d 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60955/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Donghui Xu
> 
>

Re: Splitting permissions and path updates

[Alexander Kolbasov]

That's good. Permission updates - even full permission update shouldn't be
very expensive, so we may be more willing to send one when we detect
improper holes.

On Mon, Jul 24, 2017 at 5:59 PM, Na Li  wrote:

> Sasha,
>
> Yes.
>
> SentryHDFSServiceProcessor.get_authz_updates gets perm update and path
> update separately, and puts them into the response object.
>
> Thanks,
>
> Lina
>
> On Mon, Jul 24, 2017 at 10:48 AM, Alexander Kolbasov 
> wrote:
>
> > Does it mean that without any changes the current code may send e.g. full
> > update for permissions and partial update for paths or visa versa?
> >
> > - Alex
> >
> > On Mon, Jul 24, 2017 at 5:36 PM, Na Li  wrote:
> >
> > > Sasha,
> > >
> > > When NameNode plugin asks for updates, it includes info for both
> > permission
> > > and path. However, the processing is separate. It is possible for
> Sentry
> > to
> > > send full snapshot of permission and delta change to HDFS. At Sentry,
> > perm
> > > and path processing share the same class, but they have their own
> > > instances.
> > >
> > > The current behavior can already satisfy your requirements. I have
> > > confirmed this with Sergio.
> > >
> > > You can see it in SentryPlugin.java at sentry server side.
> > >
> > >   public void initialize(Configuration conf, SentryStore sentryStore)
> > > throws SentryPluginException {
> > > PermImageRetriever permImageRetriever = new
> > > PermImageRetriever(sentryStore);
> > > PathImageRetriever pathImageRetriever = new
> > > PathImageRetriever(sentryStore);
> > > PermDeltaRetriever permDeltaRetriever = new
> > > PermDeltaRetriever(sentryStore);
> > > PathDeltaRetriever pathDeltaRetriever = new
> > > PathDeltaRetriever(sentryStore);
> > > pathsUpdater = new DBUpdateForwarder<>(pathImageRetriever,
> > > pathDeltaRetriever);   <- path has its own instance
> > > permsUpdater = new DBUpdateForwarder<>(permImageRetriever,
> > > permDeltaRetriever); <- perm has its own instance
> > > ...
> > > }
> > >
> > > Thanks,
> > >
> > > Lina
> > >
> > > On Fri, Jul 21, 2017 at 6:35 PM, Alexander Kolbasov <
> ak...@cloudera.com>
> > > wrote:
> > >
> > > > Lina’s email prompted an interesting thought - right now when the
> > > NameNode
> > > > plugin isn’t happy and wants a full update we send it both
> permissions
> > > > update and path update. Path update is very expensive while
> permissions
> > > > update is usually much smaller.
> > > >
> > > > It would be very useful to be able to send just full path update or
> > just
> > > > full perms update when there is a problem with just path or just
> > > > permissions.
> > > >
> > > > Any thoughts?
> > > >
> > > > - Alex
> > >
> >
>

Re: SENTRY-1855: PERM/PATH transactions can fail to commit to the sentry database under load

[Alexander Kolbasov]

Lina,

can you describe what problem have you discovered with approach 2.2?


On Mon, Jul 24, 2017 at 6:37 PM, Na Li  wrote:

> Sasha,
>
> I realize a serious issue in approach 2.2) that it does not function
> correctly. So I re-iterate the approaches and their pros and cons.
>
> This leaves us the option 2.1) and 2.3). The question is if the performance
> of approach 2.1) acceptable for now, and we can work on the long term
> solution 2.3) later?
>
> 2.1) Find out why we have collision on a single sentry server with
> synchronization on saving updates. Once find the cause, fix it.
> + Follow existing approach. Does not introduce big change to the code base.
> + Function correctly. There is no hole in changeID
> - Need time to investigate why synchronization at application level on a
> single sentry server does not prevent key collision.
> - Does not scale. All updates are serialized, not much concurrency.
> - Still have key collision exception and transaction failure when more than
> one Sentry servers are deployed.
> - Transaction failure at collision increases time to execute a transaction.
> - It is confusing to customer that there are transaction failure in normal
> operation. Increase support cases
>
> 2.2) Auto-increment changeID and send delta changes as much as possible
>
> The patch that achieves 5 times or more performance increase than the
> current approach.
>
> It contains the following changes
>
>- revert sentry-1795 (so the changeID is auto-increment. This avoids key
>collision. This is main reason we have performance improvement)
>- revert sentry-1824 (no need to synchronize when changeID is
>auto-increment)
>- get continuous delta list from SentryStore even when the delta list
>has hole (for example, the list is 1,2,3,5,6, return 1,2,3. If the hole
> is
>at the front of list, return full snapshot)
>
> + Relative small changes. Verified working with good performance when there
> is no transaction failure
>
> + When the hole in delta list is temporary (transaction in flight), return
> the continuous delta list is effective to deal with the hole. Most likely,
> the hole will disappear next time HDFS requests for changes.
>
> - When there is transaction failure (the hole in changeID is permanent),
> sends back full snapshot, which is expensive. If we can detect permanent
> hole, then we don't need to send full snapshot, which is very expensive,
> and may exhaust memory for big customer
>
> - When there is a temporary hole at the start of the list, even sending
> full snapshot could skip the transaction in flight. It does not function
> correctly.
>
>   For example, the list is 1,2,3,5,6, and transaction with changeID 4 is
> not done.
>
>   First, Sentry returns 1,2,3.
>
>   next round, 4 is still not done, so sentry gets list 5,6 with a hole at
> the front of the list. Sentry sends back the full snapshot with changeID 6.
>
>   then next time, HDFS will ask for changes after 6, and transaction with
> changeID 4 will be skipped even thought it may have done.
>
> 2.3) Use timestamp to sort the changes
>
> a) use timestamp, such as MSentryPermChange.createTimeMs or
> MSentryPathChange.createTimeMs to sort the entries. If there are more than
> one entry having same timestamp, use changeID to break the tie.
> b) HDFS asks for updates using these timestamp values instead of changeID.
> Sentry server sends back changes at and after this timestamp. HDFS keeps
> the list of changeIDs associated with the requesting timestamp and skip
> entries already processed. This is to handle the situation when more than
> one entry having same timestamp, and some are sent in previous request, and
> some need to be send in next request.
> c) changeID is primary key, only used to uniquely identify the entry, and
> not required to be sequential nor consecutive.
> d) Purge the change entries in DB using timestamp instead of changeID. For
> example, keep 3 polling intervals of entries to allow HDFS getting the
> changes before they are purged.
> + Sentry only sends full snapshot to HDFS at the first time when HDFS
> starts, and then always sends delta changes to HDFS. For path changes, if
> Sentry got a new full snapshot from HMS, it sends full path snapshot.
> + High concurrency. Scale well with large number of clients
> - Relative big code change for API between sentry server and sentry plugin
> at HDFS.
> - No easy way to detect that HDFS has received and processed all updates
> - If DB admin changes the DB server time backwards, this will mess up the
> ordering using timestamp
>
> Thanks,
>
> Lina
>
> On Fri, Jul 21, 2017 at 5:53 PM, Alexander Kolbasov 
> wrote:
>
> > Lina, Thank you fo

Re: Review Request 60883: SENTRY-1825: Dropping a Hive database/table doesn't cleanup the permissions associated with it

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On July 21, 2017, 8:24 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60883/
> ---
> 
> (Updated July 21, 2017, 8:24 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee 
> Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> There are several problems:
> 1) HMSFollower should read sentry-site.xml to get the authen server name, not 
> from hive-site.xml through hiveConf. The input configuration for HMSFollower 
> constructor is from sentry-site.xm. We should use that configuration to get 
> server name.
> 2) There are two variable names that could hold the value of the server. 
> "hive.sentry.server"is deprecated and is in 
> HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is
> sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We 
> should check the sentry.hive.server first. If it is not set, check the 
> deprecated hive.sentry.server to be backward compatible. If it is still not 
> set, use default value
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  e1312bf 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  547a61f 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  3d67401 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  62fde2c 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  fdf52bf 
> 
> 
> Diff: https://reviews.apache.org/r/60883/diff/6/
> 
> 
> Testing
> ---
> 
> create integration tests. reproduced the issue and verfied the fix
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: SENTRY-1855: PERM/PATH transactions can fail to commit to the sentry database under load

[Alexander Kolbasov]

Lina,

> On Jul 24, 2017, at 10:20 PM, Na Li  wrote:
> 
> Sasha,
> 
> I found the following approach that could improve the performance of the
> current approach and second approach a lot with minor code change

How do you know that it improves performance? Do you see the improvement in 
your experiments? If you do see improvements, let’s do the change.

> 
> Right now, the execution order of the transaction blocks in a transaction is
> 1. Delta transaction block to save the perm change and path change
> 2. The actual transaction
> 
> We can change the execution order to be
> 1. The actual transaction
> 2. Delta transaction block to save the perm change and path change
> 
> The benefit of this update is:
> Actual transaction has chance to fail and execution time may vary a lot.

Why would it fail? Most of our transactions fail currently because of the 
conflict on the primary key for delta updates which IMO is happening at commit 
time.

> Delta transaction has very low chance to fail and the execution time is
> near constant. As a result, the changeID is set after actual transaction is
> done.

Hmm, I don’t get that. I think changeID is modified in the same transaction, so 
the whole thing should complete.

> 
> A. If it is applied to the approach 2.1) (manually increase changeID, read
> current max value and increase by 1) , the time window exposed for key
> collision is small (the delta transaction execution time). Before, the time
> windows is the sum of delta transaction and actual transaction. So key
> collision should happen much less.

Can you run some experiments and see whether this is the case or not? This may 
or may not have some impact - difficult to tell.

Thanks,

- Sasha

> 
> B. If it is applied to the approach 2.2) (auto-increase the change ID),
> actual transaction failure does not create permanent hole in changeID. Near
> constant execution time of delta transaction makes it rare to have
> temporary hole. Without permanent hole, if there is a hole, it will be
> temporary hole. We should always wait for the hole to be filled and always
> send delta changes. If the hole is not filled, the missing transaction is
> likely purged without received by HDFS.
> 
> Thanks,
> 
> Lina
> 
> 
> On Mon, Jul 24, 2017 at 1:03 PM, Na Li  wrote:
> 
>> Approach 2.2, it uses auto-increment of changeID. When there is a
>> temporary hole at the start of the list, even sending full snapshot could
>> skip the transaction in flight. It does not function correctly.
>> 
>>  For example, the list is 1,2,3,5,6, and transaction with changeID 4 is
>> not done.
>> 
>>  First, Sentry returns 1,2,3.
>> 
>>  next round, 4 is still not done, so sentry gets list 5,6 with a hole at
>> the front of the list. Sentry sends back the full snapshot with changeID 6.
>> 
>>  then next time, HDFS will ask for changes after 6, and transaction with
>> changeID 4 will be skipped even thought it may have done.
>> 
>> On Mon, Jul 24, 2017 at 12:02 PM, Alexander Kolbasov 
>> wrote:
>> 
>>> Lina,
>>> 
>>> can you describe what problem have you discovered with approach 2.2?
>>> 
>>> 
>>> On Mon, Jul 24, 2017 at 6:37 PM, Na Li  wrote:
>>> 
>>>> Sasha,
>>>> 
>>>> I realize a serious issue in approach 2.2) that it does not function
>>>> correctly. So I re-iterate the approaches and their pros and cons.
>>>> 
>>>> This leaves us the option 2.1) and 2.3). The question is if the
>>> performance
>>>> of approach 2.1) acceptable for now, and we can work on the long term
>>>> solution 2.3) later?
>>>> 
>>>> 2.1) Find out why we have collision on a single sentry server with
>>>> synchronization on saving updates. Once find the cause, fix it.
>>>> + Follow existing approach. Does not introduce big change to the code
>>> base.
>>>> + Function correctly. There is no hole in changeID
>>>> - Need time to investigate why synchronization at application level on a
>>>> single sentry server does not prevent key collision.
>>>> - Does not scale. All updates are serialized, not much concurrency.
>>>> - Still have key collision exception and transaction failure when more
>>> than
>>>> one Sentry servers are deployed.
>>>> - Transaction failure at collision increases time to execute a
>>> transaction.
>>>> - It is confusing to customer that there are transaction failure in
>>> normal
>>>> operation. Increase support cases
>>>> 
>>>> 2.2) Auto-increment cha

Re: Review Request 61047: SENTRY-1854 HMSFollower should handle notifications even if HDFS sync is disabled.

[Alexander Kolbasov]

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




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

The name is a bit strange. From SentryStore point of view why should it 
care about hdfs sync? This is the layer that deals with perssitent storage. So 
this should be a variable that determines whether updates should be stored or 
not.



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

Can we just add a method to SentryStore that sets it and call it from 
SentryService?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 724 (original), 727 (patched)
<https://reviews.apache.org/r/61047/#comment256793>

There is a lot of duplication - many functions do that. Can you move this 
to a common code - e.g. to the execute() method?



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

Why do we need these? SentryStore may just compute one boolean and keep it 
rather then keep three separate vars.



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

Why do we need a whole gunction for that? A single boolean should be 
sufficient.


- Alexander Kolbasov


On July 21, 2017, 7:28 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61047/
> ---
> 
> (Updated July 21, 2017, 7:28 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently, Sentry does not start HMSFollower when HDFS sync is disabled. This 
> breaks PERM sync. For example, when a table/database is dropped, its 
> permission should be removed when perm sync is enabled. Without HMSFollower, 
> Sentry does not know when table/database is dropped, and therefore, cannot 
> remove the permission accordingly.
> Sentry needs to make updates to have the follow behavior
> 1) When HDFS sync is disabled + PERM sync is disabled, do not start 
> HMSFollower
> 2) when HDFS sync is enabled or PERM sync is enabled, start HMSFollower 
> (Sentry change).
> 3) when HDFS sync is enabled, we save path change and perm change. nothing 
> more. 
> 4) when perm sync is enabled, we update perm when table/database is created, 
> dropped or altered, nothing more.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  670bc5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  10d55dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  5826766 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a8ebf7c 
> 
> 
> Diff: https://reviews.apache.org/r/61047/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: SENTRY-1855: PERM/PATH transactions can fail to commit to the sentry database under load

[Alexander Kolbasov]

Lia,

I don’t understand why it would be any different, but there is no harm in 
trying. 

> On Jul 24, 2017, at 11:50 PM, Na Li  wrote:
> 
> Sasha,
> 
> "How do you know that it improves performance? Do you see the improvement
> in your experiments? If you do see improvements, let’s do the change."
> [Lina] I have not run the test. Vamsee will run the test on cluster 30
> minutes later today for current approach with re-order transactions. So we
> can see.
> 
> "Why would it fail? Most of our transactions fail currently because of the
> conflict on the primary key for delta updates which IMO is happening at
> commit time."
> [Lina] I know. We have three types of failure: whole transaction failure
> (you mentioned), delta transaction block failure, and TransactionBlock
> failure. I am looking at additional individual transaction block failure,
> besides key conflict failure of the whole transaction.



> 
> "Hmm, I don’t get that. I think changeID is modified in the same
> transaction, so the whole thing should complete."
> [Lina] I think changeID value is set in DeltaTransactionBlock after getting
> max(changeID in DB). In my proposed approach, the order of events is
> 0) transaction starts
> 1) PM executes TransactionBlock,
> 2) PM executes DeltaTransactionBlock (where changeID value is set to the
> MSentryPathChange instance or MSentryPermChange instance)
> 3) It is saved into DB at transaction commit.
> 
> In current approach, the order of events is
> 
> 0) transaction starts
> 1) PM executes DeltaTransactionBlock (where changeID value is set to the
> MSentryPathChange instance or MSentryPermChange instance)
> 2) PM executes TransactionBlock,
> 3) It is saved into DB at transaction commit.
> 
> Reducing the time between reading max(changeID in DB) and transaction
> commit will reduce the chance of key conflict. That is the whole point of
> re-order the blocks.
> 
> 
> 
> Thanks,
> 
> Lina
> 
> On Mon, Jul 24, 2017 at 3:52 PM, Alexander Kolbasov 
> wrote:
> 
>> Lina,
>> 
>>> On Jul 24, 2017, at 10:20 PM, Na Li  wrote:
>>> 
>>> Sasha,
>>> 
>>> I found the following approach that could improve the performance of the
>>> current approach and second approach a lot with minor code change
>> 
>> How do you know that it improves performance? Do you see the improvement
>> in your experiments? If you do see improvements, let’s do the change.
>> 
>>> 
>>> Right now, the execution order of the transaction blocks in a
>> transaction is
>>> 1. Delta transaction block to save the perm change and path change
>>> 2. The actual transaction
>>> 
>>> We can change the execution order to be
>>> 1. The actual transaction
>>> 2. Delta transaction block to save the perm change and path change
>>> 
>>> The benefit of this update is:
>>> Actual transaction has chance to fail and execution time may vary a lot.
>> 
>> Why would it fail? Most of our transactions fail currently because of the
>> conflict on the primary key for delta updates which IMO is happening at
>> commit time.
>> 
>>> Delta transaction has very low chance to fail and the execution time is
>>> near constant. As a result, the changeID is set after actual transaction
>> is
>>> done.
>> 
>> Hmm, I don’t get that. I think changeID is modified in the same
>> transaction, so the whole thing should complete.
>> 
>>> 
>>> A. If it is applied to the approach 2.1) (manually increase changeID,
>> read
>>> current max value and increase by 1) , the time window exposed for key
>>> collision is small (the delta transaction execution time). Before, the
>> time
>>> windows is the sum of delta transaction and actual transaction. So key
>>> collision should happen much less.
>> 
>> Can you run some experiments and see whether this is the case or not? This
>> may or may not have some impact - difficult to tell.
>> 
>> Thanks,
>> 
>> - Sasha
>> 
>>> 
>>> B. If it is applied to the approach 2.2) (auto-increase the change ID),
>>> actual transaction failure does not create permanent hole in changeID.
>> Near
>>> constant execution time of delta transaction makes it rare to have
>>> temporary hole. Without permanent hole, if there is a hole, it will be
>>> temporary hole. We should always wait for the hole to be filled and
>> always
>>> send delta changes. If the hole is not filled, the missing transaction is
>>> likely purged without

Re: SENTRY-1855: PERM/PATH transactions can fail to commit to the sentry database under load

[Alexander Kolbasov]

> 
> Reducing the time between reading max(changeID in DB) and transaction
> commit will reduce the chance of key conflict. That is the whole point of
> re-order the blocks.
> 


Why would this affect anything? Whenever you read max(changeID) inside a 
transaction you should get exactly the same value since we are using 
repeatable-read transaction isolation level. You will get the value at the 
start of the transaction, not at the time you read it.

Re: Review Request 61047: SENTRY-1854 HMSFollower should handle notifications even if HDFS sync is disabled.

[Alexander Kolbasov]

> On July 24, 2017, 8:57 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 724 (original), 727 (patched)
> > <https://reviews.apache.org/r/61047/diff/1/?file=1780874#file1780874line727>
> >
> > There is a lot of duplication - many functions do that. Can you move 
> > this to a common code - e.g. to the execute() method?
> 
> Na Li wrote:
> If I do it in execute(), it means all callers will create 
> DeltaTransactionBlock() unnecessary if hdfssync is disabled.

Can you refactor execute() to do this?


- Alexander


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


On July 21, 2017, 7:28 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61047/
> ---
> 
> (Updated July 21, 2017, 7:28 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently, Sentry does not start HMSFollower when HDFS sync is disabled. This 
> breaks PERM sync. For example, when a table/database is dropped, its 
> permission should be removed when perm sync is enabled. Without HMSFollower, 
> Sentry does not know when table/database is dropped, and therefore, cannot 
> remove the permission accordingly.
> Sentry needs to make updates to have the follow behavior
> 1) When HDFS sync is disabled + PERM sync is disabled, do not start 
> HMSFollower
> 2) when HDFS sync is enabled or PERM sync is enabled, start HMSFollower 
> (Sentry change).
> 3) when HDFS sync is enabled, we save path change and perm change. nothing 
> more. 
> 4) when perm sync is enabled, we update perm when table/database is created, 
> dropped or altered, nothing more.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  670bc5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  10d55dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  5826766 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a8ebf7c 
> 
> 
> Diff: https://reviews.apache.org/r/61047/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests
> 
> 
> Thanks,
> 
> Na Li
> 
>

Review Request 61113: SENTRY-1755 Add HMSFollower per-operation metrics

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1755 Add HMSFollower per-operation metrics


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
 1f34d05d855152726444f064076b60e2b1b4a8f7 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61113: SENTRY-1755 Add HMSFollower per-operation metrics

[Alexander Kolbasov]

> On July 25, 2017, 4:48 p.m., Na Li wrote:
> > some of your code change in SENTRY-1696 ( 
> > https://reviews.apache.org/r/60806/diff/1#1) is lost in HMSFollower due to 
> > HMS follower refector. We don't measure how long does it take to get full 
> > snapshot any more. Can you add it back in this Jira?

Can you clarify what is lost? If it is lost, I'll handle it in a separate JIRA.


- Alexander


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


On July 25, 2017, 4:32 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61113/
> ---
> 
> (Updated July 25, 2017, 4:32 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1755
> https://issues.apache.org/jira/browse/SENTRY-1755
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1755 Add HMSFollower per-operation metrics
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  1f34d05d855152726444f064076b60e2b1b4a8f7 
> 
> 
> Diff: https://reviews.apache.org/r/61113/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 61113: SENTRY-1755 Add HMSFollower per-operation metrics

[Alexander Kolbasov]

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




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

a) This is enum
b) This is existing code I just refactored it a tiny bit


- Alexander Kolbasov


On July 25, 2017, 4:32 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61113/
> ---
> 
> (Updated July 25, 2017, 4:32 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1755
> https://issues.apache.org/jira/browse/SENTRY-1755
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1755 Add HMSFollower per-operation metrics
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
>  1f34d05d855152726444f064076b60e2b1b4a8f7 
> 
> 
> Diff: https://reviews.apache.org/r/61113/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Review Request 61144: SENTRY-1869 Try to use pool with idle connections first

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1869 Try to use pool with idle connections first


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java
 04a515af6f2eac35cb91a24b3a58df5fa2797cf3 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61138: Sentry HA- SENTRY-1867 - DataNucleus.Query INFO level logging spams Sentry log files

[Alexander Kolbasov]

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



I think the right way to fix it is by applying SENTRY-1868 rather then 
modifying DataNucleus debug levels.

- Alexander Kolbasov


On July 26, 2017, 2:44 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61138/
> ---
> 
> (Updated July 26, 2017, 2:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> For Sentry HA - In the Sentry service, DataNucleus.Query property, when not 
> defined in log4j.properties file, is by default set to INFO. This floods the 
> log files with INFO level logs every few seconds. We should set the default 
> value to WARN to avoid this, unless until specified in the log4j.properties 
> file
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java
>  3a981b2a1 
> 
> 
> Diff: https://reviews.apache.org/r/61138/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: DISCUSSION: Prepare master for next release (2.0.0)

[Alexander Kolbasov]

+1. Important note - anyone who is working on Sentry will need to get a
fresh clone.

On Wed, Jul 26, 2017 at 10:57 PM, Sergio Pena 
wrote:

> Hi,
>
> Now that branch-1.8 has been created, we need to prepare the current master
> branch for the next major release. However, the current master won't be
> used for that, and the new sentry-ha-redesign branch will become master.
>
> We'd like to do this as soon as possible. This change will be disruptive,
> and all contributors will have to update their master with the new one.
>
> Can we do this today or tomorrow? Anytime no later than Friday so that we
> have the most updated master to the community.
>
> - Sergio
>

Re: Review Request 60955: SENTRY-1853: Add the log level access mechanism

[Alexander Kolbasov]

> On July 24, 2017, 3:54 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
> > Lines 32 (patched)
> > <https://reviews.apache.org/r/60955/diff/1/?file=1778962#file1778962line32>
> >
> > Why is the size specified in the code? Is it the standard practice?
> 
> Donghui Xu wrote:
> I have tested. When the length of input text is 50, user can enter 45 
> characters which meet the requirements.

We can enter class names here - do we know that all class names fit 45 
characters?


- Alexander


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


On July 26, 2017, 2:28 a.m., Donghui Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60955/
> ---
> 
> (Updated July 26, 2017, 2:28 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: SENTRY-1853
> https://issues.apache.org/jira/browse/SENTRY-1853
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add the log level setting and reading mechanism, through which users can 
> control the log content.
> This mechanism provides the url interface,through which users can dynamically 
> set the log level. Service restores the original log level after restart.
> 
> The format of the interface is as follows:
> 1. Read the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME
> 2. Set the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME & level = LOG_LEVEL
> 
> The above parameters include:
> HOST: host name or ip address of sentry server.
> PORT: port of sentry server.
> CLASS_NAME: name of class whose log level is read or set.
> LOG_LEVEL: the log level to be set.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
>  8b4d374 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60955/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Donghui Xu
> 
>

Re: Review Request 60955: SENTRY-1853: Add the log level access mechanism

[Alexander Kolbasov]

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 61 (patched)
<https://reviews.apache.org/r/60955/#comment257137>

Please specify under what conditions null is returned.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 82 (patched)
<https://reviews.apache.org/r/60955/#comment257138>

missing line



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 83 (patched)
<https://reviews.apache.org/r/60955/#comment257139>

"Handle" is a generic term, adding a line or two about what it actually 
does is more helpful.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 101 (patched)
<https://reviews.apache.org/r/60955/#comment257140>

Do we need to return some kind of HTTP error in this case?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
Lines 105 (patched)
<https://reviews.apache.org/r/60955/#comment257141>

Here you are sending inputs to outputs as is which may be a subject to 
cross-scripting vulnerabilities. You need to url-escape it - see SENTRY-1665 
for an example.



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
Lines 61 (patched)
<https://reviews.apache.org/r/60955/#comment257142>

Please document what is the test case actually testing like

- Send level and class via the HTTP interface and verify that it is set at 
the loogger.


- Alexander Kolbasov


On July 26, 2017, 2:28 a.m., Donghui Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60955/
> ---
> 
> (Updated July 26, 2017, 2:28 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: SENTRY-1853
> https://issues.apache.org/jira/browse/SENTRY-1853
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add the log level setting and reading mechanism, through which users can 
> control the log content.
> This mechanism provides the url interface,through which users can dynamically 
> set the log level. Service restores the original log level after restart.
> 
> The format of the interface is as follows:
> 1. Read the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME
> 2. Set the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME & level = LOG_LEVEL
> 
> The above parameters include:
> HOST: host name or ip address of sentry server.
> PORT: port of sentry server.
> CLASS_NAME: name of class whose log level is read or set.
> LOG_LEVEL: the log level to be set.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
>  8b4d374 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60955/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Donghui Xu
> 
>

Review Request 61187: SENTRY-1868 SentryStore should set loadResultsAtCommit to false when query result isn't needed

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1868 SentryStore should set loadResultsAtCommit to false when query 
result isn't needed


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 670bc5e29bed4f8f14b7f9a75a86934c6e4ac5b4 


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


Testing
---


Thanks,

Alexander Kolbasov

Sentry wiki updates

[Alexander Kolbasov]

What is the established process for updating Sentry wiki? Do we just give
permissions to people to do that or there is some kind of review process?

How was it handled so far?

- Alex

Access to the sentry git repository

[Alexander Kolbasov]

Hello,

as we discussed earlier we need to delete current master which is now 1.8.0
and create a new master from sentry-ha-redesign.

Since git doesn't support remote renames, the only way to do this is to
delete master branch, but this requires changing
'receive.denyDeleteCurrent' pproperty *on the server*.

Is there a way to set this property either via some web interface or by
logging to an appropriate host?

- Alex

Re: Review Request 61047: SENTRY-1854 HMSFollower should handle notifications even if HDFS sync is disabled.

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
Line 68 (original), 69 (patched)
<https://reviews.apache.org/r/61047/#comment257203>

This file deals with Generic permissions model which is never subject to 
HDFS sync.



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

This is unused import



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

This is a very confusing name What is saveDeltaBlockEnabled? It should be 
at least documented, but a better name would be good.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 245 (original), 247 (patched)
<https://reviews.apache.org/r/61047/#comment257212>

I thnik we should have a simple constructor with old signature for all 
users who don't care about this.

Also, enableSaveDeltaBlock is rather bad name - what is "SaveDeltaBlock"? 
You never explain what it is.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 4089 (original), 4090 (patched)
<https://reviews.apache.org/r/61047/#comment257215>

Please update the comment to reflect the changed behavior.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 4096 (original), 4097 (patched)
<https://reviews.apache.org/r/61047/#comment257216>

This is outside the scope but since you are touching the function - it 
makes sense to change to 

tbs = new ArrayList(2);

otherwise we allocate space for 10 elements. Not a big deal, but still...



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 4098 (original), 4101 (patched)
<https://reviews.apache.org/r/61047/#comment257217>

Why not just 

`tbs.add(new DeltaTransactionBlock(update));`



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java
Line 117 (original), 118 (patched)
<https://reviews.apache.org/r/61047/#comment257204>

WHy have it here? For the purpose of this servlet it doesn't matter whether 
HDFS sync is enabled or not.



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

This function isn't needed, instead you need a boolean (that replaces 
hdfsSyncEnabled and syncPolicyStore) that is true when HMSFollower is enabled. 
hdfsSYncEnabled and syncPolicyStore are only used to compute this, so they can 
be local variables instead.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
Lines 225 (patched)
<https://reviews.apache.org/r/61047/#comment257210>

typo: policy, not pollicy


- Alexander Kolbasov


On July 27, 2017, 3:57 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61047/
> -------
> 
> (Updated July 27, 2017, 3:57 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently, Sentry does not start HMSFollower when HDFS sync is disabled. This 
> breaks PERM sync. For example, when a table/database is dropped, its 
> permission should be removed when perm sync is enabled. Without HMSFollower, 
> Sentry does not know when table/database is dropped, and therefore, cannot 
> remove the permission accordingly.
> Sentry needs to make updates to have the follow behavior
> 1) When HDFS sync is disabled + PERM sync is disabled, do not start 
> HMSFollower
> 2) when HDFS sync is enabled or PERM sync is enabled, start HMSFollower 
> (Sentry change).
> 3) when HDFS sync is enabled, we save path change and perm change. nothing 
> more. 
> 4) when perm sync is enabled, we update perm when table/database is created, 
> dropped or altered, nothing more.
> 
> 
> Diffs
> -
> 
>   
&g

Re: Review Request 61047: SENTRY-1854 HMSFollower should handle notifications even if HDFS sync is disabled.

[Alexander Kolbasov]

> On July 27, 2017, 9:52 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Line 68 (original), 69 (patched)
> > <https://reviews.apache.org/r/61047/diff/3/?file=1784509#file1784509line69>
> >
> > This file deals with Generic permissions model which is never subject 
> > to HDFS sync.
> 
> Na Li wrote:
> Initially, I have SentryStore(Configuration conf), and caller does not 
> need to care about HDFS sync. SentryStore finds out through configuration. 
> You did not want SentryStore to figure it out. So I changed the constructor 
> to take it as input.
> 
> Now, you don't want caller to provide the value. What should I do?
> 
> Do you mean I should provide two constructors for SentryStore, 
> SentryStore(Configuration conf) (SentryStore finds out the value from 
> configuration) and SentryStore(Configuration conf, boolean saveDeltaChange)? 
> 
> Can you be more specific about what you want? Otherwise, it will take too 
> long to close this issue.

Option 1) Have two constructors for SentryStore. One constructor just accepts 
conf and always disables saving permission deltas. Another constructor has a 
parameter that tells whether it should be enabled or not. This cnstructor 
should be only used for Policy (Hive) permissions.

Option 2) Do not change constructor, but add a setter in SentryStore which sets 
the boolean value asking it to persist permission deltas. This setter should 
only be called by policy store engine. This is probably best.


> On July 27, 2017, 9:52 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 171 (patched)
> > <https://reviews.apache.org/r/61047/diff/3/?file=1784510#file1784510line171>
> >
> > This is a very confusing name What is saveDeltaBlockEnabled? It should 
> > be at least documented, but a better name would be good.
> 
> Na Li wrote:
> What is the name you prefer?

How about 'persistUpdateDeltas' ?


- Alexander


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


On July 27, 2017, 3:57 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61047/
> ---
> 
> (Updated July 27, 2017, 3:57 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently, Sentry does not start HMSFollower when HDFS sync is disabled. This 
> breaks PERM sync. For example, when a table/database is dropped, its 
> permission should be removed when perm sync is enabled. Without HMSFollower, 
> Sentry does not know when table/database is dropped, and therefore, cannot 
> remove the permission accordingly.
> Sentry needs to make updates to have the follow behavior
> 1) When HDFS sync is disabled + PERM sync is disabled, do not start 
> HMSFollower
> 2) when HDFS sync is enabled or PERM sync is enabled, start HMSFollower 
> (Sentry change).
> 3) when HDFS sync is enabled, we save path change and perm change. nothing 
> more. 
> 4) when perm sync is enabled, we update perm when table/database is created, 
> dropped or altered, nothing more.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  4cb46ab 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  670bc5e 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java
>  8a8bbd3 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  10d55dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java
>  5826766 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  82f600b 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a8ebf7c 
>   
&

Re: Access to the sentry git repository

[Alexander Kolbasov]

This seems to be the only available option if we can't delete master. But
this will create merge commit. So far we were avoiding merge commits, but
it may be ok in this case. What do you guys think - is it ok to introduce
merge commit to move sentry-ha-redesign to master?

On Fri, Jul 28, 2017 at 10:56 AM, Colm O hEigeartaigh 
wrote:

> Instead of deleting the branch, we could look at doing something like the
> answer suggested here:
>
> https://stackoverflow.com/questions/2862590/how-to-
> replace-master-branch-in-git-entirely-from-another-branch
>
> git checkout sentry-ha-redesign
> git merge -s ours master
> git checkout master
> git merge sentry-ha-redesign
>
> Colm.
>
>
> On Thu, Jul 27, 2017 at 7:50 PM, Alexander Kolbasov 
> wrote:
>
> > Hello,
> >
> > as we discussed earlier we need to delete current master which is now
> 1.8.0
> > and create a new master from sentry-ha-redesign.
> >
> > Since git doesn't support remote renames, the only way to do this is to
> > delete master branch, but this requires changing
> > 'receive.denyDeleteCurrent' pproperty *on the server*.
> >
> > Is there a way to set this property either via some web interface or by
> > logging to an appropriate host?
> >
> > - Alex
> >
>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Access to the sentry git repository

[Alexander Kolbasov]

The merge commit preserves all the history from both existing master and
sentry-ha-redesign branch.

On Fri, Jul 28, 2017 at 6:27 PM, Sergio Pena 
wrote:

> Does the merge commit still leaves the history of sentry-ha-redesign? Or it
> wraps all those commits into one single merge commit?
>
> If we do leave the history, then it would be fine to do it.
>
> On Fri, Jul 28, 2017 at 11:23 AM, Alexander Kolbasov 
> wrote:
>
> > This seems to be the only available option if we can't delete master. But
> > this will create merge commit. So far we were avoiding merge commits, but
> > it may be ok in this case. What do you guys think - is it ok to introduce
> > merge commit to move sentry-ha-redesign to master?
> >
> > On Fri, Jul 28, 2017 at 10:56 AM, Colm O hEigeartaigh <
> cohei...@apache.org
> > >
> > wrote:
> >
> > > Instead of deleting the branch, we could look at doing something like
> the
> > > answer suggested here:
> > >
> > > https://stackoverflow.com/questions/2862590/how-to-
> > > replace-master-branch-in-git-entirely-from-another-branch
> > >
> > > git checkout sentry-ha-redesign
> > > git merge -s ours master
> > > git checkout master
> > > git merge sentry-ha-redesign
> > >
> > > Colm.
> > >
> > >
> > > On Thu, Jul 27, 2017 at 7:50 PM, Alexander Kolbasov <
> ak...@cloudera.com>
> > > wrote:
> > >
> > > > Hello,
> > > >
> > > > as we discussed earlier we need to delete current master which is now
> > > 1.8.0
> > > > and create a new master from sentry-ha-redesign.
> > > >
> > > > Since git doesn't support remote renames, the only way to do this is
> to
> > > > delete master branch, but this requires changing
> > > > 'receive.denyDeleteCurrent' pproperty *on the server*.
> > > >
> > > > Is there a way to set this property either via some web interface or
> by
> > > > logging to an appropriate host?
> > > >
> > > > - Alex
> > > >
> > >
> > >
> > >
> > > --
> > > Colm O hEigeartaigh
> > >
> > > Talend Community Coder
> > > http://coders.talend.com
> > >
> >
>

Sentry master is moved to sentry-ha-redesign branch

[Alexander Kolbasov]

Hello everyone.

The sentry master is now merged with sentry-ha-redesign branch. The merge
replaced all the conflicts with the version of sentry-ha-redesign.

Please stop using sentry-ha-redesign branch and continue working on master
for all 2.0.0 work.

Best,

- Alex Kolbasov.

Re: Sentry master is moved to sentry-ha-redesign branch

[Alexander Kolbasov]

Please note that this didn't automatically bring commits missing in
sentry-ha-redesign branch but present in master - these should still be
ported.


On Fri, Jul 28, 2017 at 7:09 PM, Alexander Kolbasov 
wrote:

> Hello everyone.
>
> The sentry master is now merged with sentry-ha-redesign branch. The merge
> replaced all the conflicts with the version of sentry-ha-redesign.
>
> Please stop using sentry-ha-redesign branch and continue working on master
> for all 2.0.0 work.
>
> Best,
>
> - Alex Kolbasov.
>

Re: Sentry Wiki Permissions

[Alexander Kolbasov]

I tried to add you as a contributor in JIRA but can't find you by your ID.

On Wed, Aug 2, 2017 at 4:50 PM, Coral Waters  wrote:

> Hi,
> I'm a technical writer working with the Sentry Cloudera team and I'd like
> to make changes to the documentation wiki. Can someone give me permissions
> on the wiki? Are there any processes I should be aware of for
> editing/adding content? My jira username is coral.w
>
> Thanks,
> Coral
>

Re: [VOTE] Release Sentry version 1.8.0

[Alexander Kolbasov]

+1

Alex

> On Aug 3, 2017, at 12:16, Colm O hEigeartaigh  wrote:
> 
> +1.
> 
> Colm.
> 
> On Wed, Aug 2, 2017 at 1:46 PM, Sergio Pena 
> wrote:
> 
>> This is the release of Apache Sentry, version 1.8.0.
>> It fixes the following issues:
>> https://issues.apache.org/jira/projects/SENTRY/versions/12335076
>> 
>> Source files : https://dist.apache.org/repos/dist/dev/sentry/1.8.0/
>> 
>> Tag to be voted on
>> https://git-wip-us.apache.org/repos/asf/sentry/?p=sentry.
>> git;a=commit;h=release-1.8.0
>> Sentry's KEYS containing the PGP key we used to sign the release:
>> https://people.apache.org/keys/group/sentry.asc (or
>> http://www.apache.org/dist/sentry/KEYS for non committers)
>> 
>> Note that this is a source only release and we are voting on the source:
>> tag=release-1.8.0, SHA=32d85bf2cc265dc33596a53d49b558bf20131480 (You can
>> get the hash of the tag by doing "git rev-list release-1.8.0 | head -n 1" )
>> 
>> Vote will be open for 72 hours.
>> 
>> [ ] +1 approve
>> [ ] +0 no opinion
>> [ ] -1 disapprove (and reason why)
>> 
>> 
>> Thanks,
>> - Sergio Pena
>> 
> 
> 
> 
> -- 
> Colm O hEigeartaigh
> 
> Talend Community Coder
> http://coders.talend.com

Re: [ANNOUNCE] Apache Sentry 1.8.0 released

[Alexander Kolbasov]

Cool! Thanks Sergio!

On Mon, Aug 7, 2017 at 2:26 PM, Sergio Pena 
wrote:

> The Apache Sentry team is happy to announce the release of version 1.8.0.
> Apache Sentry is a system to enforce fine grained role based authorization
> to data and metadata stored on a Hadoop cluster.
>
> The release bits are available at:
> http://sentry.apache.org/general/downloads.html
>
> Sentry 1.8.0 Release Notes are available here:
> https://issues.apache.org/jira/secure/ReleaseNote.jspa?
> projectId=12314720&version=12335076
>
> We would like to thank all contributors who made the release possible.
>
> Regards,
> Sentry team
>

Re: Maven artifacts for 1.8.0

[Alexander Kolbasov]

Thanks Colm!

On Tue, Aug 8, 2017 at 2:48 AM, Colm O hEigeartaigh 
wrote:

> I created some Maven artifacts for 1.8.0:
>
> https://repository.apache.org/content/repositories/orgapachesentry-1003/
>
> Please take a look and let me know if there are any objections, I will
> publish them later this week if I don't hear anything.
>
> Colm.
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Review Request 61499: SENTRY-1873 - Upgrade PMD plugin and fix related issues

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Aug. 8, 2017, 3:06 p.m., Colm O hEigeartaigh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61499/
> ---
> 
> (Updated Aug. 8, 2017, 3:06 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: SENTRY-1873
> https://issues.apache.org/jira/browse/SENTRY-1873
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This task is to update the PMD plugin and fix the related issues. We have 
> some unused methods + imports that are not being flagged by the current 
> plugin version, as well as a few other issues.
> 
> 
> Diffs
> -
> 
>   build-tools/sentry-pmd-ruleset.xml 8a264469 
>   pom.xml 5a06a41c 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  d6516918 
>   
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java
>  f5d44317 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  73239ee7 
>   
> sentry-policy/sentry-policy-indexer/src/main/java/org/apache/sentry/policy/indexer/IndexerWildcardPrivilege.java
>  10e34965 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
>  c221c348 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java
>  18be0fa6 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  a38772ba 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  d20de265 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java
>  17dc13f1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  6e228754 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
>  eb39b0db 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java
>  4ee1597b 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  859a9fea 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java
>  1b732da1 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  6820a9b3 
>   
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/component/SecureRealTimeGetComponent.java
>  1ac9ccc1 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java
>  63b00e22 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestDbHdfsBase.java
>  17bc6120 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestDbHdfsExtMaxGroups.java
>  5784d857 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestDbHdfsMaxGroups.java
>  6dd03d5e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  6ea67631 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/TestHiveWarehouseOnExtFs.java
>  31000d4e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestSentryListenerSentryDeserializer.java
>  fe72b91d 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/tools/CreateSentryTestScaleData.java
>  cd9fa282 
> 
> 
> Diff: https://reviews.apache.org/r/61499/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colm O hEigeartaigh
> 
>

Review Request 61537: SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 5af1e4ff36d53e399f1f51b023aaf379ebf62b92 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 b0a202eef391accce3cea5e62374acf37e3d1b38 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4f76a94c9c97f9ad8ab491f8ad444e80c30e258e 


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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61537: SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot

[Alexander Kolbasov]

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

(Updated Aug. 9, 2017, 11:50 p.m.)


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


Changes
---

Addressed code review comments from Lina and Vamsee


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


Repository: sentry


Description
---

SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 5af1e4ff36d53e399f1f51b023aaf379ebf62b92 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 b0a202eef391accce3cea5e62374acf37e3d1b38 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4f76a94c9c97f9ad8ab491f8ad444e80c30e258e 


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

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


Testing
---


Thanks,

Alexander Kolbasov

Re: Review Request 61537: SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot

[Alexander Kolbasov]

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

(Updated Aug. 10, 2017, 6:43 a.m.)


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


Changes
---

Added some simple unit tests.


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


Repository: sentry


Description
---

SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 5af1e4ff36d53e399f1f51b023aaf379ebf62b92 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 b0a202eef391accce3cea5e62374acf37e3d1b38 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4f76a94c9c97f9ad8ab491f8ad444e80c30e258e 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 PRE-CREATION 


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

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


Testing (updated)
---

Added unit tests (not yet available for all operations)


Thanks,

Alexander Kolbasov

[URGENT] Broken master

[Alexander Kolbasov]

I noticed that our recent builds on master are failing with

/home/jenkins/jenkins-slave/workspace/PreCommit-SENTRY-Build/sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java:64:
error: no suitable constructor found for
TAuthzUpdateRequest(int,int,int)
TAuthzUpdateRequest updateRequest = new TAuthzUpdateRequest(1, 1, 0);
^
constructor TAuthzUpdateRequest.TAuthzUpdateRequest() is not applicable
  (actual and formal argument lists differ in length)
constructor
TAuthzUpdateRequest.TAuthzUpdateRequest(long,long,long,String,String)
is not applicable
  (actual and formal argument lists differ in length)
constructor
TAuthzUpdateRequest.TAuthzUpdateRequest(TAuthzUpdateRequest) is not
applicable
  (actual and formal argument lists differ in length)


Someone committed the change without going through Jenkins build and
broke master buid.


This should be taken care of ASAP.


Alex.

PMD violation caused by too many static imports

[Alexander Kolbasov]

I recently saw PMD violation caused by too many static imports. I think
this one should be disabled. Colm - can you tweak PMD configuration for
that?

Thanks,

- Alex

Re: [URGENT] Broken master

[Alexander Kolbasov]

Sorry, false alarm. It was one specific build that is broken, not a master
problem!

On Thu, Aug 10, 2017 at 7:52 AM, Alexander Kolbasov 
wrote:

> I noticed that our recent builds on master are failing with
>
> /home/jenkins/jenkins-slave/workspace/PreCommit-SENTRY-Build/sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java:64:
>  error: no suitable constructor found for TAuthzUpdateRequest(int,int,int)
> TAuthzUpdateRequest updateRequest = new TAuthzUpdateRequest(1, 1, 0);
> ^
> constructor TAuthzUpdateRequest.TAuthzUpdateRequest() is not applicable
>   (actual and formal argument lists differ in length)
> constructor 
> TAuthzUpdateRequest.TAuthzUpdateRequest(long,long,long,String,String) is not 
> applicable
>   (actual and formal argument lists differ in length)
> constructor TAuthzUpdateRequest.TAuthzUpdateRequest(TAuthzUpdateRequest) 
> is not applicable
>   (actual and formal argument lists differ in length)
>
>
> Someone committed the change without going through Jenkins build and broke 
> master buid.
>
>
> This should be taken care of ASAP.
>
>
> Alex.
>
>

Re: PMD violation caused by too many static imports

[Alexander Kolbasov]

Cool, thanks!

> On Aug 10, 2017, at 08:21, Colm O hEigeartaigh  wrote:
> 
> Yes, done.
> 
> Colm.
> 
> On Thu, Aug 10, 2017 at 4:04 PM, Alexander Kolbasov 
> wrote:
> 
>> I recently saw PMD violation caused by too many static imports. I think
>> this one should be disabled. Colm - can you tweak PMD configuration for
>> that?
>> 
>> Thanks,
>> 
>> - Alex
>> 
> 
> 
> 
> -- 
> Colm O hEigeartaigh
> 
> Talend Community Coder
> http://coders.talend.com

Permissions to edit wiki page for Sentry

[Alexander Kolbasov]

Does anyone know how to give someone permissions to edit Sentry wiki pages?

Thanks,

- Alex

Re: Permissions to edit wiki page for Sentry

[Alexander Kolbasov]

Thanks Colm!

On Thu, Aug 10, 2017 at 9:16 AM, Colm O hEigeartaigh 
wrote:

> You can do it here, but make sure they have a CLA filed first!
>
> https://cwiki.apache.org/confluence/spaces/spacepermissions.action?key=
> SENTRY
>
> Colm.
>
> On Thu, Aug 10, 2017 at 5:06 PM, Alexander Kolbasov 
> wrote:
>
> > Does anyone know how to give someone permissions to edit Sentry wiki
> pages?
> >
> > Thanks,
> >
> > - Alex
> >
>
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Re: Review Request 61537: SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot

[Alexander Kolbasov]

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

(Updated Aug. 11, 2017, 12:28 a.m.)


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


Changes
---

- Addressed code review feedback from Brian
- Added unit tests for all operations for FullUpdateModifier
- Fixed issues found by unit test


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


Repository: sentry


Description
---

SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 5af1e4ff36d53e399f1f51b023aaf379ebf62b92 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 b0a202eef391accce3cea5e62374acf37e3d1b38 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4f76a94c9c97f9ad8ab491f8ad444e80c30e258e 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61537/diff/4/

Changes: https://reviews.apache.org/r/61537/diff/3-4/


Testing (updated)
---

Added unit tests for FullUpdateModifier.


Thanks,

Alexander Kolbasov

Re: Review Request 61537: SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot

[Alexander Kolbasov]

> On Aug. 10, 2017, 3:32 p.m., Brian Towles wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
> > Lines 204-205 (patched)
> > <https://reviews.apache.org/r/61537/diff/3/?file=1794568#file1794568line204>
> >
> > Empty catch...  do we want to log or something?

Sure, will log a message here.


- Alexander


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


On Aug. 10, 2017, 6:43 a.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61537/
> ---
> 
> (Updated Aug. 10, 2017, 6:43 a.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vamsee Yarlagadda.
> 
> 
> Bugs: SENTRY-1874
> https://issues.apache.org/jira/browse/SENTRY-1874
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1874 Do not require quiet HMS when taking initial HMS snapshot
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  5af1e4ff36d53e399f1f51b023aaf379ebf62b92 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  b0a202eef391accce3cea5e62374acf37e3d1b38 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4f76a94c9c97f9ad8ab491f8ad444e80c30e258e 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateModifier.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61537/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit tests (not yet available for all operations)
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: multiple definition of unused image number

[Alexander Kolbasov]

One of this is part of thrift definitions:

# Value used to specify that a path image number is not used on a
request or response
const i64 UNUSED_PATH_UPDATE_IMG_NUM = -1;

The default value is -1

Since you raised the question - can you investigate this deeper?


Thanks,


- Sasha


On Thu, Aug 10, 2017 at 2:18 PM, Na Li  wrote:

> Hi,
>
> I found  multiple definitions of unused image number.
>
> Can we just remove
> "sentry_hdfs_serviceConstants.UNUSED_PATH_UPDATE_IMG_NUM" and keep
> "ServiceConstants.IMAGE_NUMBER_UPDATE_UNINITIALIZED"?
>
> public class sentry_hdfs_serviceConstants {
>
>   public static final long UNUSED_PATH_UPDATE_IMG_NUM = -1L;
> }
>
> ServiceConstants.IMAGE_NUMBER_UPDATE_UNINITIALIZED = 0L;
>
> Thanks,
>
> Lina
>

Re: Review Request 60955: SENTRY-1853: Add the log level access mechanism

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On July 29, 2017, 5:59 a.m., Donghui Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60955/
> ---
> 
> (Updated July 29, 2017, 5:59 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Bugs: SENTRY-1853
> https://issues.apache.org/jira/browse/SENTRY-1853
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add the log level setting and reading mechanism, through which users can 
> control the log content.
> This mechanism provides the url interface,through which users can dynamically 
> set the log level. Service restores the original log level after restart.
> 
> The format of the interface is as follows:
> 1. Read the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME
> 2. Set the log level. URL address is http: //HOST: PORT/logLevel? Log = 
> CLASS_NAME & level = LOG_LEVEL
> 
> The above parameters include:
> HOST: host name or ip address of sentry server.
> PORT: port of sentry server.
> CLASS_NAME: name of class whose log level is read or set.
> LOG_LEVEL: the log level to be set.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/LogLevelServlet.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
>  8b4d374 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerLogLevel.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60955/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Donghui Xu
> 
>

Something strange with Sentry builds

[Alexander Kolbasov]

I noticed a weird Jenkins behavior - my patches are not triggering build.
It looks like they are submitted to a queue and then very quickly disappear.

Is it Jenkins getting tired of me or others experienced similar issues?

- Alex

Isssues with Jenkins - builds are not triggered

[Alexander Kolbasov]

I filed  https://issues.apache.org/jira/browse/INFRA-14845 to track this.

Re: Review Request 61598: SENTRY-1856: Persisting HMS snapshot and the notification-id to database in same transaction

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Aug. 11, 2017, 7:12 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61598/
> ---
> 
> (Updated Aug. 11, 2017, 7:12 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar 
> kalvagadda, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> when saving full snapshot, save its latest notification ID as well
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4305691 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  b0a202e 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ccfb5ab 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  d44212f 
> 
> 
> Diff: https://reviews.apache.org/r/61598/diff/1/
> 
> 
> Testing
> ---
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 61821: SENTRY-1897: Rename sentry property to provide the list of sentry servers

[Alexander Kolbasov]

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



Would it make sense to still handle the old config if it is present and the new 
one isn't?

- Alexander Kolbasov


On Aug. 22, 2017, 6:57 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61821/
> ---
> 
> (Updated Aug. 22, 2017, 6:57 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and 
> Na Li.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1897: Rename sentry property to provide the list of sentry servers
> 
> 
> Diffs
> -
> 
>   conf/sentry-site.xml.hive-client.example 
> c9f1d0588c99c7815c5a7f35382e32ef73116e78 
>   conf/sentry-site.xml.hive-client.template 
> becff9c6aa8c2ea5afa6979f271ec61e79b36c1f 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
>  fd07887963b4ac0f6e1243df774c87e843fed29b 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  f99301008ca1a0fd9efce4131c037436dccfdb2c 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
>  5b6ef53b8f79d64c0f791fabb6b6fee86c895c67 
>   
> sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
>  c19ccbd12fde3567496b6847cf2b67b3c49cd655 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  e62c54a5c0a932617a88492944a6d9fb76571903 
> 
> 
> Diff: https://reviews.apache.org/r/61821/diff/1/
> 
> 
> Testing
> ---
> 
> Will make sure all the unit tests work.
> 
> 
> Thanks,
> 
> Vamsee Yarlagadda
> 
>

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

[Alexander Kolbasov]

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



Overall looks good. This are partial comments - I still need more time to go 
through all the changes. These comments are all nits.

Can you add a block comment at the top of HMSPaths.java explaining the trickery 
and the motivation for using trees rather then HashSet.


sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 166 (original), 166 (patched)
<https://reviews.apache.org/r/61827/#comment259592>

Do we need a List here or we can use Collection? We just use it to walk 
entries.



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

Didn't you insert the element already on line 229  above?



sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Line 502 (original), 530 (patched)
<https://reviews.apache.org/r/61827/#comment259598>

This can be package-private



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

It may be easier to read (and make SonarLint happy) if you extract the 
ternary operation in a seperate statement. We don't need to save memory on Java 
lines :-)



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

This can be package-private



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

Same comment here about ternary op.


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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




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

Note that in many cases we can just get all roles (or a useful subset) in 
memory and do some joins ourselves in memory.



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

Problem with direct SQL is that it should be carefully tested with each DB 
engine. It would be better to stay within JDOQL.



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

You are introducing SQL injection vulnerability here. Please take a look at 
QueryParamBuilder or provide parameterized query.


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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



This is the last comment I have - looks good!


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

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


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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

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


- Alexander


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


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

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

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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



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


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

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



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

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



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

Would VARCHAR2 work here?


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1822 Allow multiple Sentry reporters.


Diffs
-

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


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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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



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


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

Is this a stray change?



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

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



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

s/will be/is/



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

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


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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



For MySQL I see this:

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

Does it match your changes?

- Alexander Kolbasov


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

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

[Alexander Kolbasov]

> On Aug. 28, 2017, 5:16 p.m., Alexander Kolbasov wrote:
> > For MySQL I see this:
> > 
> > ALTER TABLE `SENTRY_DB_PRIVILEGE`
> >   ADD UNIQUE `SENTRY_DB_PRIV_PRIV_NAME_UNIQ` 
> > (`SERVER_NAME`,`DB_NAME`,`TABLE_NAME`,`COLUMN_NAME`,`URI`(250),`ACTION`,`WITH_GRANT_OPTION`);
> > 
> > Does it match your changes?
> 
> Na Li wrote:
> Yes. Before this change, both the unique index defined in jdo and the 
> unique index defined in SQL are inserted in the database table. After this 
> change, the unique index in datanucleus (jdo) is removed, there will be only 
> one unique index in that table.

I see two differences - the sql script uses URI with the lengh 250, while you 
use 128. And sql scripts does use unique on a bunch of fields which you do 
remove in package.jdo file. SHouldn't these two match each other?


- Alexander


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


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

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

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


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

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



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

Since hashCode incorporates the rest we can just return true here


- Alexander Kolbasov


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

Blog entry for Sentry 1.8 release

[Alexander Kolbasov]

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

- Alex.

Re: Blog entry for Sentry 1.8 release

[Alexander Kolbasov]

I don't know how blog access is controlled. Does anyone know? Can you retry
now since you are added as committer? May by you have access now?

On Tue, Aug 29, 2017 at 1:49 PM, Sergio Pena 
wrote:

> I would like to, but I need a PMC person to give me permissions to write a
> blog. I signed-up on the blog but I don't see any place to write a new
> entry.
>
> Could you give me permissions?
>
> On Tue, Aug 29, 2017 at 3:37 PM, Alexander Kolbasov 
> wrote:
>
> > Sergio - would you like to write a blog entry for the 1.8 release? I
> think
> > others can help by pointing out major things that were added in 1.8 that
> > they know about.
> >
> > - Alex.
> >
>

Re: [DISCUSSION] Move to JDK8 and Datanucleus 4

[Alexander Kolbasov]

+1 for both.

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

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

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

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

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

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


Diffs
-

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


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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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

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


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


Changes
---

Addressed code review comment from Misha.


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


Repository: sentry


Description
---

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


Diffs (updated)
-

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


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

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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1903 TransactionManager shows retried transactions starting from 0


Diffs
-

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


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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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



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


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

How are you handling InterruptedException here?


- Alexander Kolbasov


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

Re: [DISCUSSION] Move to JDK8 and Datanucleus 4

[Alexander Kolbasov]

We are not actively release maintenance releases, but companies who use
Sentry do. I suggest to split the discussion into two separate threads -
one is about moving to JDK8 and a separate one about actually allowing JDK8
specific features because these are very different decisions with different
consequences.

On Wed, Aug 30, 2017 at 7:07 AM, Sergio Pena 
wrote:

> Agree with Colm.
>
> We're not active on releasing maintenance releases (something to consider
> by the way) and the current Sentry 2.0 development is redesigning code that
> it might not work for older releases.
>
> On Wed, Aug 30, 2017 at 4:24 AM, Colm O hEigeartaigh 
> wrote:
>
> > +1 to switch to JDK 8 + Datanucleus 4.
> >
> > Alex, I'm just wondering how likely it is that we will run into these
> > backporting scenarios? If I look at the past releases I don't see too
> many
> > minor releases:
> >
> > [DIR] 1.2.0-incubating/   2016-04-06 17:49-
> > [DIR] 1.3.0-incubating/   2016-04-06 17:49-
> > [DIR] 1.4.0-incubating/   2016-04-06 17:49-
> > [DIR] 1.5.1-incubating/   2016-04-06 17:49-
> > [DIR] 1.6.0-incubating/   2016-04-06 17:49-
> > [DIR] 1.7.0/  2017-06-26 18:46-
> > [DIR] 1.8.0/
> >
> > It seems a bit limiting not to allow lambdas to me given that we don't
> > maintain active older release branches.
> >
> > Colm.
> >
> > On Wed, Aug 30, 2017 at 12:34 AM, Alexander Kolbasov  >
> > wrote:
> >
> > > +1 for both.
> > >
> > > I would be still against actually allowing Java 8 specific code since
> > > people may want to backport fixes to old releases which might still use
> > > Java 7. Allowing Java-8 exclusive features may make this process
> > > complicated. Lambdas are cute, but we can do without them for a while.
> It
> > > would be good to have some automatic enforcement so that Java8 features
> > do
> > > not leak in accidentally.
> > >
> > > On Tue, Aug 29, 2017 at 3:56 PM, Na Li  wrote:
> > >
> > > > +1 for moving to datanucleus 4 and Java 8
> > > >
> > > > On Tue, Aug 29, 2017 at 3:48 PM, Sergio Pena <
> sergio.p...@cloudera.com
> > >
> > > > wrote:
> > > >
> > > > > Hi All,
> > > > >
> > > > > There is a JIRA request (SENTRY-1893
> > > > > <https://issues.apache.org/jira/browse/SENTRY-1893>) looking to
> add
> > > > > support
> > > > > for JDK8 and make it as the default and minimum JDK version to use.
> > > JDK7
> > > > > has reached the end of life, and many other Apache components
> > > (including
> > > > > Hive 2.1) have already switched to JDK8 as the minimum version.
> > > > >
> > > > > I would like to use this email thread to discuss if we should
> follow
> > > this
> > > > > trend and switch to JDK8 on our current Sentry 2.0 development.
> > Moving
> > > to
> > > > > JDK8 will also allow us to use the new API that brings, such as
> > lambda
> > > > > functions.
> > > > >
> > > > > Also, in order to support JDK8, we should switch to Datanucleus 4
> > > because
> > > > > Datanucleus 3 have some problems with it. Datanucleus 3 is also
> old,
> > > and
> > > > > other components already switched to version 4 as well. We already
> > have
> > > > > support for version 4, so the question here is if we should drop
> > > > > Datanucleus 3 support and just use version 4 as the default.
> > > > >
> > > > > Sentry 2.0 is our current major version development, so it makes
> > sense
> > > to
> > > > > do this move in this version.
> > > > >
> > > > > What do you all think?
> > > > >
> > > > > - Sergio
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Colm O hEigeartaigh
> >
> > Talend Community Coder
> > http://coders.talend.com
> >
>

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

[Alexander Kolbasov]

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

Oh, ok. no problem then.


- Alexander


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


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

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

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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




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

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



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

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



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

This should be HiveCOnnectionFactory as well



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

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



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

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



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

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

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



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

So you decided not to log anything in this case?



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

can be final



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

can be final



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

can use HashSet<>



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

this and all other methods can be package-private



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

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

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

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

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


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

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


Diffs
-

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


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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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

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


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

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


- Alexander


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


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

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

[Alexander Kolbasov]

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

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


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


Changes
---

Fixed SentryStore tests


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


Repository: sentry


Description
---

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


Diffs (updated)
-

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


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

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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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

(Updated Aug. 31, 2017, 4:14 p.m.)


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


Changes
---

Fixed additional SentryStore tests that depend on ordering


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


Repository: sentry


Description
---

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


Diffs (updated)
-

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


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

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


Testing
---


Thanks,

Alexander Kolbasov

Draft of the quarterly ASF report for Apache Sentry

[Alexander Kolbasov]

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

Thanks,

- Alex.

## Description:

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

## Issues:

There are no issues requiring board attention at this time

## Activity:

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

## Health report:

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

## PMC changes:

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

## Committer base changes:

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

## Releases:

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

## Mailing list activity:

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

## JIRA activity:

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

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

[Alexander Kolbasov]

Sergio, can you share Apache Hive experience with this issue?

Thanks,

- Alex

On Thu, Aug 31, 2017 at 1:10 PM, Sergio Pena 
wrote:

> Hi All,
>
> This thread discussion is a follow-up to the old thread related to
> supporting JDK8 and Datanucleus 4 as the minimum version for Sentry 2.0.
> This is dedicated to discuss whether we should allow using JDK8 specific
> features (such as lambda functions and other useful API) in Sentry.
>
> Advantages are that we could start using these cool features that come in
> JDK8 and forget about JDK7 for all.
>
> Disadvantages are that doing backports on older releases and/or allowing
> other companies backporting fixes from Sentry 2.x will make these backports
> harder because JDK7 is still in use.
>
> Maintenance releases are not active but companies are still pretty active
> on Sentry.
>
> Questions to answer:
> - What should we do?
> - If we decide to keep JDK7 compatibility, how long should we keep this
> until we move completely to JDK8?
>
> In my opinion, companies will always be outdated with what we do as an
> Apache community. Taking a look at what we are doing with SentryHA
> redesign, this is a breaking change for companies too because fixes on this
> new design may not work for other companies and/or the backports could be
> harder.
>
> However, the current Sentry 2.0 has been active with JDK7 as the support,
> and users active on 2.0 may be using JDK7 environments only.
>
> So,
> Should we wait until Sentry 2.1 or newer releases to allow JDK8 features?
> Should we start in Sentry 2.0?
>
> Any thoughts?
>
> - Sergio
>

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

[Alexander Kolbasov]

> On Aug. 31, 2017, 12:25 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java
> > Lines 127 (patched)
> > <https://reviews.apache.org/r/61973/diff/1/?file=1807071#file1807071line127>
> >
> > If I am reading this code correctly, the following will happen:
> > 
> > 1) Every time we request IDs starting from last seen ID, so normally 
> > the first ID in the sequence is always a duplicate
> > 2) This first ID in the sequence isn't in the cache yet, so we go to 
> > the sentryStore and fetch the hash and compare.
> > 
> > This means that we always go to sentryStore for the frist element of 
> > the batch which means every time we call fetchNotifications().
> > 
> > This is not very bad, but if we store the sha1 hash of the last event 
> > in the cache we don't have to go to sentryStore at all under normal 
> > circumstances
> 
> Sergio Pena wrote:
> Keeping the cache of the last ID is not enough. The 
> HiveNotificationFetcher does not know if the IDs fetched are processed, so 
> the next call to fetchNotifications() may  be called with an ID that was not 
> cached. If I want to cache all of them, then I will add more procesisng on 
> getting the sha1() of all the notifications and adding them to the cache. 
> Isn't simpler just to keep it the way it is now?

This is possible but requires a different code structure. This fix is good 
enough.


- Alexander


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


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

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

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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




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

Can we use

pm.setDetachAllOnCommit(false);

here?



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

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



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

Do we need results of the query after close?



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

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



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

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



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

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



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

Just use HashSet<>(result)



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

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



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

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



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

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



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

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


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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




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

Please add comment explaining what this test case is testing.


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

> On Sept. 1, 2017, 7 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java
> > Line 142 (original), 143 (patched)
> > <https://reviews.apache.org/r/62007/diff/3/?file=1808686#file1808686line143>
> >
> > Applicable to more than one place in both FullUpdateModifier and 
> > FullUpdateInitializer: seems odd that first Set is replaced with 
> > Collection, only to follow by downcasting Collection back to Set. Why not 
> > to have method signatures keep the actual type? Replacing subclass with 
> > su[erclass makes sense when either a) you trully do not need subclass, so 
> > you just keep your code more generic, or b) the code of the method can 
> > downcast to more than one subclass, depending on the circumstances. Neither 
> > seems to apply. Besides, cast exception is always cleaner to be thrown up 
> > the call chain.
> 
> Vadim Spector wrote:
> Even if its justified, then perhaps add little javadoc explaining this?

The problem is that `Map>` is not compatible with 
`Map>`. That's why I can't keep the original type.


- Alexander


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


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

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

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

SENTRY-1917: Sentry should work around HIVE-16994


Diffs
-

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


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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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


Fix it, then Ship it!




Ship It!


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

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

pathRetrieverIsBusy = newAtomicBoolean(true)

This will express the meaning better IMO.


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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

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


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


Repository: sentry


Description
---

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


Diffs
-

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


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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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

(Updated Sept. 6, 2017, 12:54 a.m.)


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


Changes
---

Addressed comments from Vamsee - now only filter on the outbound path


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


Repository: sentry


Description
---

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


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 55092eec04d86b64aa91e30e76bb63c40b2a375b 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
 69e43fd651d8952f8a614e25e664a86bd1ba0248 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 551f184c35c8c3bf4e4b49aa540a55b016bb9a32 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 478ccf9bb983dcb9d022eb03df8b90ba197fafb0 
  
sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
 acf1cf5e2a9ba1cfaef9ccc404e56791c10b4be9 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 63e718cd403ee3bbfa62ab572ac678b418b82cc7 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
 a8d70c11c879828d16f44df2168853a5b709c8f7 


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

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


Testing
---


Thanks,

Alexander Kolbasov

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

[Alexander Kolbasov]

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




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

Are you sure you get your boolean logic right?

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

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



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

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



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

I think this can be package-private



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

What if this was already set?



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

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



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

Ditto



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

This can be simplified to

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

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

In Java you can't say

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

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



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

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


- Alexander Kolbasov


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

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

[Alexander Kolbasov]

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




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

The condition can change right after we read it but probably it is Ok in 
this case.



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

Remove else - previous clause ends with return



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

"A full update is running loading" - this is a bit confusing. Can you 
refrase the message?



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

We check for the condition twice which makes logic a bit convoluted. Can 
you refactor it to check only once?

if (curImgNum > imgNum) {
if (full_update) {
  return empty list
}
return full image
}



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

Should we add a log here as well?



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

The way you are using it volatile is good enough, but Atomic works as well.



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

The first sentence should end with dot.
return what?

May be just 

@return True if collecting full HMS snapshot is in progress



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

Can you add check (with preconditions) that it is set to false at the 
beginning - like an assert statement?



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

This is unused import



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

should this be static as well?
Do we need to be concerned with concurrency?


- Alexander Kolbasov


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