Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On Nov. 21, 2017, 7:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63993/
> ---
> 
> (Updated Nov. 21, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
> persisted. When initially HDFS Sync is disabled, we will create a full 
> snapshot but never persist it, and at the same time populate 
> SENTRY_HMS_NOTIFICATION_ID table. Later when HDFS sync is enabled, a full 
> snapshot will not occur unless until HMS and Sentry are out of sync. This 
> will result in ACL's not being applied unless until HMS and Sentry are out of 
> sync, since we only send NN snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values 
> greater than 0
> We should create a full snapshot if hdfsSync is enabled, and 
> AUTHZ_PATHS_SNAPSHOT_ID is empty
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4dc2bf6d1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  bbcf09382 
> 
> 
> Diff: https://reviews.apache.org/r/63993/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test
> 
> Second patch:
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test -Dtest=TestHMSFollower
> 
> 
> File Attachments
> 
> 
> SENTRY-2046.04.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/11/21/c86539e8-54bd-4b73-8e4d-62512520d4a1__SENTRY-2046.04.patch
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Arjun Mishra via Review Board]

> On Nov. 21, 2017, 8:41 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 257 (patched)
> > 
> >
> > the logging level can be info to be consistent with another logging 
> > level.

Sergio brought up a good point where if the Sentry service was started and no 
DDL or DML queries are being executed, then MAuthzPathsSnapshotId table will 
still be empty, because we will be fetching empty notificaitons. In that case a 
INFO level log can result in spamming.


- Arjun


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


On Nov. 21, 2017, 7:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63993/
> ---
> 
> (Updated Nov. 21, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
> persisted. When initially HDFS Sync is disabled, we will create a full 
> snapshot but never persist it, and at the same time populate 
> SENTRY_HMS_NOTIFICATION_ID table. Later when HDFS sync is enabled, a full 
> snapshot will not occur unless until HMS and Sentry are out of sync. This 
> will result in ACL's not being applied unless until HMS and Sentry are out of 
> sync, since we only send NN snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values 
> greater than 0
> We should create a full snapshot if hdfsSync is enabled, and 
> AUTHZ_PATHS_SNAPSHOT_ID is empty
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4dc2bf6d1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  bbcf09382 
> 
> 
> Diff: https://reviews.apache.org/r/63993/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test
> 
> Second patch:
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test -Dtest=TestHMSFollower
> 
> 
> File Attachments
> 
> 
> SENTRY-2046.04.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/11/21/c86539e8-54bd-4b73-8e4d-62512520d4a1__SENTRY-2046.04.patch
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Na Li via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 248 (original), 250 (patched)


can log a message in this case at info level. So when a full snapshot is 
requested, we know the reason. Getting full snapshot should be rare, so we can 
log at info level.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 257 (patched)


the logging level can be info to be consistent with another logging level.


- Na Li


On Nov. 21, 2017, 7:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63993/
> ---
> 
> (Updated Nov. 21, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
> persisted. When initially HDFS Sync is disabled, we will create a full 
> snapshot but never persist it, and at the same time populate 
> SENTRY_HMS_NOTIFICATION_ID table. Later when HDFS sync is enabled, a full 
> snapshot will not occur unless until HMS and Sentry are out of sync. This 
> will result in ACL's not being applied unless until HMS and Sentry are out of 
> sync, since we only send NN snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values 
> greater than 0
> We should create a full snapshot if hdfsSync is enabled, and 
> AUTHZ_PATHS_SNAPSHOT_ID is empty
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4dc2bf6d1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  bbcf09382 
> 
> 
> Diff: https://reviews.apache.org/r/63993/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test
> 
> Second patch:
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test -Dtest=TestHMSFollower
> 
> 
> File Attachments
> 
> 
> SENTRY-2046.04.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/11/21/c86539e8-54bd-4b73-8e4d-62512520d4a1__SENTRY-2046.04.patch
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On Nov. 21, 2017, 7:56 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63993/
> ---
> 
> (Updated Nov. 21, 2017, 7:56 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
> persisted. When initially HDFS Sync is disabled, we will create a full 
> snapshot but never persist it, and at the same time populate 
> SENTRY_HMS_NOTIFICATION_ID table. Later when HDFS sync is enabled, a full 
> snapshot will not occur unless until HMS and Sentry are out of sync. This 
> will result in ACL's not being applied unless until HMS and Sentry are out of 
> sync, since we only send NN snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values 
> greater than 0
> We should create a full snapshot if hdfsSync is enabled, and 
> AUTHZ_PATHS_SNAPSHOT_ID is empty
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4dc2bf6d1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  bbcf09382 
> 
> 
> Diff: https://reviews.apache.org/r/63993/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test
> 
> Second patch:
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test -Dtest=TestHMSFollower
> 
> 
> File Attachments
> 
> 
> SENTRY-2046.04.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/11/21/c86539e8-54bd-4b73-8e4d-62512520d4a1__SENTRY-2046.04.patch
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Arjun Mishra via Review Board]

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

(Updated Nov. 21, 2017, 7:54 p.m.)


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


Changes
---

Accounting for Sergio's changes


Repository: sentry


Description
---

AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
persisted. When initially HDFS Sync is disabled, we will create a full snapshot 
but never persist it, and at the same time populate SENTRY_HMS_NOTIFICATION_ID 
table. Later when HDFS sync is enabled, a full snapshot will not occur unless 
until HMS and Sentry are out of sync. This will result in ACL's not being 
applied unless until HMS and Sentry are out of sync, since we only send NN 
snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values greater than 0
We should create a full snapshot if hdfsSync is enabled, and 
AUTHZ_PATHS_SNAPSHOT_ID is empty


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 4dc2bf6d1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c4cc91806 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
 bbcf09382 


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


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml  test

Second patch:
mvn -f sentry-provider/sentry-provider-db/pom.xml  test -Dtest=TestHMSFollower


File Attachments (updated)


SENTRY-2046.04.patch
  
https://reviews.apache.org/media/uploaded/files/2017/11/21/c86539e8-54bd-4b73-8e4d-62512520d4a1__SENTRY-2046.04.patch


Thanks,

Arjun Mishra

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Arjun Mishra via Review Board]

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

(Updated Nov. 21, 2017, 6:35 p.m.)


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


Repository: sentry


Description
---

AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
persisted. When initially HDFS Sync is disabled, we will create a full snapshot 
but never persist it, and at the same time populate SENTRY_HMS_NOTIFICATION_ID 
table. Later when HDFS sync is enabled, a full snapshot will not occur unless 
until HMS and Sentry are out of sync. This will result in ACL's not being 
applied unless until HMS and Sentry are out of sync, since we only send NN 
snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values greater than 0
We should create a full snapshot if hdfsSync is enabled, and 
AUTHZ_PATHS_SNAPSHOT_ID is empty


Diffs
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 4dc2bf6d1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c4cc91806 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
 bbcf09382 


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


Testing (updated)
---

mvn -f sentry-provider/sentry-provider-db/pom.xml  test

Second patch:
mvn -f sentry-provider/sentry-provider-db/pom.xml  test -Dtest=TestHMSFollower


Thanks,

Arjun Mishra

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Arjun Mishra via Review Board]

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

(Updated Nov. 21, 2017, 6:34 p.m.)


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


Changes
---

Accounted for Lina and Kalyan's comments


Repository: sentry


Description
---

AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
persisted. When initially HDFS Sync is disabled, we will create a full snapshot 
but never persist it, and at the same time populate SENTRY_HMS_NOTIFICATION_ID 
table. Later when HDFS sync is enabled, a full snapshot will not occur unless 
until HMS and Sentry are out of sync. This will result in ACL's not being 
applied unless until HMS and Sentry are out of sync, since we only send NN 
snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values greater than 0
We should create a full snapshot if hdfsSync is enabled, and 
AUTHZ_PATHS_SNAPSHOT_ID is empty


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 4dc2bf6d1 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c4cc91806 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
 bbcf09382 


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

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


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml  test


Thanks,

Arjun Mishra

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[kalyan kumar kalvagadda via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 218 (patched)


Please update the log. "Force a full snapshot" is not accurate here as 
there are no snpshots taken so far.
It may be something like "Need to request a full HMS snapshot"


- kalyan kumar kalvagadda


On Nov. 21, 2017, 5:21 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63993/
> ---
> 
> (Updated Nov. 21, 2017, 5:21 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
> persisted. When initially HDFS Sync is disabled, we will create a full 
> snapshot but never persist it, and at the same time populate 
> SENTRY_HMS_NOTIFICATION_ID table. Later when HDFS sync is enabled, a full 
> snapshot will not occur unless until HMS and Sentry are out of sync. This 
> will result in ACL's not being applied unless until HMS and Sentry are out of 
> sync, since we only send NN snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values 
> greater than 0
> We should create a full snapshot if hdfsSync is enabled, and 
> AUTHZ_PATHS_SNAPSHOT_ID is empty
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4dc2bf6d1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  bbcf09382 
> 
> 
> Diff: https://reviews.apache.org/r/63993/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63993: SENTRY-2046: Create a full snapshot if AUTHZ_PATHS_SNAPSHOT_ID is empty, even if HMS and Sentry Notifications are in sync

[Na Li via Review Board]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 217 (patched)


can you add this logic inside the function isFullSnapshotRequired()? In 
this way, we can see when to get full snapshot in one place.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 238 (original), 246 (patched)


update the comments to match the logic of the code


- Na Li


On Nov. 21, 2017, 5:21 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63993/
> ---
> 
> (Updated Nov. 21, 2017, 5:21 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> AUTHZ_PATHS_SNAPSHOT_ID is only populated when a full HMS snapshot is 
> persisted. When initially HDFS Sync is disabled, we will create a full 
> snapshot but never persist it, and at the same time populate 
> SENTRY_HMS_NOTIFICATION_ID table. Later when HDFS sync is enabled, a full 
> snapshot will not occur unless until HMS and Sentry are out of sync. This 
> will result in ACL's not being applied unless until HMS and Sentry are out of 
> sync, since we only send NN snapshots if AUTHZ_PATHS_SNAPSHOT_ID has values 
> greater than 0
> We should create a full snapshot if hdfsSync is enabled, and 
> AUTHZ_PATHS_SNAPSHOT_ID is empty
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  4dc2bf6d1 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
>  bbcf09382 
> 
> 
> Diff: https://reviews.apache.org/r/63993/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml  test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>