Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

2018-10-16 Thread Arjun Mishra via Review Board 

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

(Updated Oct. 16, 2018, 9:55 p.m.)


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


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


Repository: sentry


Description
---

Process of persisting the snapshot might take longer based on the snapshot 
size. It would be helpfull to to be able to understand where the sentry sentry 
stands in the process of persisting.

 

Adding a Metric for this will be helpfull in debugging.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 d34340cd6 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 b6dca7aa8 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 7a736ca96 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 3a68eb6bf 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 059621a68 


Diff: https://reviews.apache.org/r/68958/diff/6/

Changes: https://reviews.apache.org/r/68958/diff/5-6/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

2018-10-16 Thread Arjun Mishra via Review Board 

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

(Updated Oct. 16, 2018, 9:54 p.m.)


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


Changes
---

Post feedback


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


Repository: sentry


Description
---

Process of persisting the snapshot might take longer based on the snapshot 
size. It would be helpfull to to be able to understand where the sentry sentry 
stands in the process of persisting.

 

Adding a Metric for this will be helpfull in debugging.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
 d2d85d3a2 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

2018-10-16 Thread Arjun Mishra via Review Board 


> On Oct. 16, 2018, 3:25 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3460-3462 (patched)
> > 
> >
> > Could you display the progress in %? Use the objectsPersistedCount for 
> > that.
> > 
> > Btw, sorry I told you about using HMS snapshot message, but this method 
> > references to that as 'Full paths', so it would be good to make that change 
> > to be consistent.

I think counts are better than percentages as they are accurate while 
percentages will have to be rounded up or down. Also percentages can be easily 
calculated


- Arjun


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


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69030: SENTRY-2427: Use Hadoop KerberosName class to derive shortName

2018-10-16 Thread Na Li via Review Board 

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


Ship it!




Ship It!

- Na Li


On Oct. 16, 2018, 4:17 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69030/
> ---
> 
> (Updated Oct. 16, 2018, 4:17 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2427
> https://issues.apache.org/jira/browse/SENTRY-2427
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry doesn't use auth to local group mapping hadoop configuration. We may 
> have a use case for cross realm users to have access to sentry service and in 
> which case Sentry needs to have access to those configurations. Switching to 
> using KerberosName will handle that case and other cases as well
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
>  d2d85d3a2 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69030/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

2018-10-16 Thread Arjun Mishra via Review Board 


> On Oct. 16, 2018, 3:25 p.m., Sergio Pena wrote:
> > Could you test this log message and paste the real messages here?

Sergio, this is how the real message will look like:

2018-10-16 12:43:00,322 INFO org.apache.sentry.service.thrift.HMSFollower: 
Persisting full snapshot for notification Id = 111800. Number of authorization 
objects = 9
2018-10-16 12:43:00,346 INFO 
org.apache.sentry.provider.db.service.persistent.SentryStore: HMS snapshot 
persisting progress: authz_objs_persisted=0 authz_paths_persisted=0 
authz_objs_size=9 authz_paths_size=14
2018-10-16 12:43:00,430 INFO 
org.apache.sentry.provider.db.service.persistent.SentryStore: HMS snapshot 
persisting progress: authz_objs_persisted=0 authz_paths_persisted=0 
authz_objs_size=9 authz_paths_size=14
2018-10-16 12:43:00,441 INFO 
org.apache.sentry.provider.db.service.persistent.SentryStore: HMS snapshot 
persisting progress: authz_objs_persisted=2 authz_paths_persisted=2 
authz_objs_size=9 authz_paths_size=14
2018-10-16 12:43:00,451 INFO 
org.apache.sentry.provider.db.service.persistent.SentryStore: HMS snapshot 
persisting progress: authz_objs_persisted=4 authz_paths_persisted=5 
authz_objs_size=9 authz_paths_size=14
2018-10-16 12:43:00,465 INFO 
org.apache.sentry.provider.db.service.persistent.SentryStore: HMS snapshot 
persisting progress: authz_objs_persisted=5 authz_paths_persisted=6 
authz_objs_size=9 authz_paths_size=14
2018-10-16 12:43:00,485 INFO 
org.apache.sentry.provider.db.service.persistent.SentryStore: HMS snapshot 
persisting progress: authz_objs_persisted=8 authz_paths_persisted=9 
authz_objs_size=9 authz_paths_size=14


- Arjun


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


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69030: SENTRY-2427: Use Hadoop KerberosName class to derive shortName

2018-10-16 Thread Arjun Mishra via Review Board 

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

(Updated Oct. 16, 2018, 4:17 p.m.)


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


Changes
---

Some hadoop versions don't throw NoMatchingRuleException


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


Repository: sentry


Description
---

Sentry doesn't use auth to local group mapping hadoop configuration. We may 
have a use case for cross realm users to have access to sentry service and in 
which case Sentry needs to have access to those configurations. Switching to 
using KerberosName will handle that case and other cases as well


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/GSSCallback.java
 d2d85d3a2 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestGSSCallback.java
 PRE-CREATION 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

2018-10-16 Thread Sergio Pena via Review Board 

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



Could you test this log message and paste the real messages here?


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3436-3439 (patched)


Initialize these variables before the setDetachAllOnCommit() and make more 
space lines to make the code more readable.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3442 (patched)


isn't lastProgressTime a better name than initialTime? I initially thought 
it was the first time it was taken, but then I see this is updated on every 
print.

I don't like to make comments on variables names, but this name does not 
correspond to the actual use of the variable.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3444-3449 (patched)


Move the logging block after the objects has been persisted and the 
counters updated so that it displays the actual completion of the process.

It will be likely that the log message will never be 100% because it is 
printed before the actual persisting process.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3460-3462 (patched)


Could you display the progress in %? Use the objectsPersistedCount for that.

Btw, sorry I told you about using HMS snapshot message, but this method 
references to that as 'Full paths', so it would be good to make that change to 
be consistent.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3463 (patched)


Agree, the timestamp is already part of the log message, so this is just 
duplicating a value.


- Sergio Pena


On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 12, 2018, 2 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7a736ca96 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>