Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[kalyan kumar kalvagadda via Review Board]

> On Sept. 12, 2018, 1:57 p.m., kalyan kumar kalvagadda wrote:
> > I'm reviewing the code. Just thinking if this logic can be simplified. I 
> > will put more comments later today.
> 
> Sergio Pena wrote:
> What's wrong with my logic? The code works, all test cases are working 
> too.

I did not it's wrong. Before commenting I was thinking if it could be 
simplified.


- kalyan kumar


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


On Sept. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68689/
> ---
> 
> (Updated Sept. 11, 2018, 8:56 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2398
> https://issues.apache.org/jira/browse/sentry-2398
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
> where the order of the upgrades found in the upgrade.order. file is not 
> the correct order.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  0d4f26c6d47991313794d250e5c232262c7559b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68689/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Na Li via Review Board]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 137 (patched)


This condition can be changed to
if(invalidateCacheOnSendingDeltas && imageCache.hasCache())

as long as imageCache has cache, we should invalidate it. It has nothing to 
do with if the cache is current or not.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathsUpdateImageCache.java
Lines 24 (patched)


this should be

PathsUpdateImageCache implements ImageCache



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathsUpdateImageCache.java
Lines 56 (patched)


this should be
if(pathsUpdateImageCache.getSeqNum() < sequenceNum)

When system starts, NN will ask with sequenceNum = 0, and full snapshot 
will have pathsUpdateImageCache.getSeqNum() >= 0. 

Have you tested that the cache mechanism does work as expected? Will 
isImageCurrent() always return false even when cache is up-to-date?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermissionsUpdateImageCache.java
Lines 24 (patched)


same problem



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermissionsUpdateImageCache.java
Lines 50 (patched)


same issue


- Na Li


On Sept. 12, 2018, 6:41 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68547/
> ---
> 
> (Updated Sept. 12, 2018, 6:41 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2370
> https://issues.apache.org/jira/browse/SENTRY-2370
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When NN requests path updates from sentry and if it exceeds the time 
> threshold, on consecutive attempts sentry will attempt to fetch the full 
> update from scratch. Instead it should cache it and update the cache before 
> sending it to NN
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
>  2d21411e2 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  08b16a4df 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathsUpdateImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermissionsUpdateImageCache.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  b8f5ce7db 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
>  f86ce6f83 
> 
> 
> Diff: https://reviews.apache.org/r/68547/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Sept. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68689/
> ---
> 
> (Updated Sept. 11, 2018, 8:56 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2398
> https://issues.apache.org/jira/browse/sentry-2398
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
> where the order of the upgrades found in the upgrade.order. file is not 
> the correct order.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  0d4f26c6d47991313794d250e5c232262c7559b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68689/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68547: SENTRY-2370: Create a cache of PathUpdates to send to NN

[Arjun Mishra via Review Board]

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

(Updated Sept. 12, 2018, 6:41 p.m.)


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


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


Repository: sentry


Description
---

When NN requests path updates from sentry and if it exceeds the time threshold, 
on consecutive attempts sentry will attempt to fetch the full update from 
scratch. Instead it should cache it and update the cache before sending it to NN


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageCache.java
 PRE-CREATION 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 2d21411e2 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 08b16a4df 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathsUpdateImageCache.java
 PRE-CREATION 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermissionsUpdateImageCache.java
 PRE-CREATION 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 b8f5ce7db 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java
 f86ce6f83 


Diff: https://reviews.apache.org/r/68547/diff/5/

Changes: https://reviews.apache.org/r/68547/diff/4-5/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[Sergio Pena via Review Board]

> On Sept. 12, 2018, 1:57 p.m., kalyan kumar kalvagadda wrote:
> > I'm reviewing the code. Just thinking if this logic can be simplified. I 
> > will put more comments later today.

What's wrong with my logic? The code works, all test cases are working too.


- Sergio


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


On Sept. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68689/
> ---
> 
> (Updated Sept. 11, 2018, 8:56 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2398
> https://issues.apache.org/jira/browse/sentry-2398
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
> where the order of the upgrades found in the upgrade.order. file is not 
> the correct order.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  0d4f26c6d47991313794d250e5c232262c7559b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68689/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68689: SENTRY-2398: Support multiple target versions on single source versions during schema upgrades

[kalyan kumar kalvagadda via Review Board]

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



I'm reviewing the code. Just thinking if this logic can be simplified. I will 
put more comments later today.

- kalyan kumar kalvagadda


On Sept. 11, 2018, 8:56 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68689/
> ---
> 
> (Updated Sept. 11, 2018, 8:56 p.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2398
> https://issues.apache.org/jira/browse/sentry-2398
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add support on the SentrySchemaTool to allow more complex upgrade scenarios 
> where the order of the upgrades found in the upgrade.order. file is not 
> the correct order.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java
>  0d4f26c6d47991313794d250e5c232262c7559b5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryUpgradeOrder.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryUpgradeOrder.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68689/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>