Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

> On July 24, 2018, 7:55 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 304-312 (patched)
> > 
> >
> > I don't recommend this log message. 
> > 
> > Logging the names of databases, tables and partition names may not be 
> > good idea as that could be huge information.
> > 
> > 
> > Let's handle the logging for FullUpdateInitializer.java seperate jira. 
> > Let's use the jir SENTRY-2301 for it.
> 
> Arjun Mishra wrote:
> Ok I'll modify it and upload a new patch
> 
> kalyan kumar kalvagadda wrote:
> Arjun, I gave a +2 if the above comment is addressed.
> 
> Arjun Mishra wrote:
> I saw that. Thanks. Uploaded a new patch just for records

Removed any logs pertaining to PARTITIONS since we can have millions and so the 
log string will be too big which may even cause OOM


- Arjun


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


On July 25, 2018, 3:32 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 25, 2018, 3:32 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  61b0d10ca 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  992d8ab52 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  12266cb25 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 25, 2018, 3:32 p.m.)


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


Changes
---

Post feedback and test failures


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 61b0d10ca 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 992d8ab52 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 12266cb25 


Diff: https://reviews.apache.org/r/67879/diff/10/

Changes: https://reviews.apache.org/r/67879/diff/9-10/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

> On July 24, 2018, 7:55 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 304-312 (patched)
> > 
> >
> > I don't recommend this log message. 
> > 
> > Logging the names of databases, tables and partition names may not be 
> > good idea as that could be huge information.
> > 
> > 
> > Let's handle the logging for FullUpdateInitializer.java seperate jira. 
> > Let's use the jir SENTRY-2301 for it.
> 
> Arjun Mishra wrote:
> Ok I'll modify it and upload a new patch
> 
> kalyan kumar kalvagadda wrote:
> Arjun, I gave a +2 if the above comment is addressed.

I saw that. Thanks. Uploaded a new patch just for records


- Arjun


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


On July 20, 2018, 3:44 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 20, 2018, 3:44 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  61b0d10ca 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  992d8ab52 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  12266cb25 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[kalyan kumar kalvagadda via Review Board]

> On July 24, 2018, 7:55 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 304-312 (patched)
> > 
> >
> > I don't recommend this log message. 
> > 
> > Logging the names of databases, tables and partition names may not be 
> > good idea as that could be huge information.
> > 
> > 
> > Let's handle the logging for FullUpdateInitializer.java seperate jira. 
> > Let's use the jir SENTRY-2301 for it.
> 
> Arjun Mishra wrote:
> Ok I'll modify it and upload a new patch

Arjun, I gave a +2 if the above comment is addressed.


- kalyan kumar


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


On July 20, 2018, 3:44 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 20, 2018, 3:44 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  61b0d10ca 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  992d8ab52 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  12266cb25 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

> On July 24, 2018, 7:55 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 304-312 (patched)
> > 
> >
> > I don't recommend this log message. 
> > 
> > Logging the names of databases, tables and partition names may not be 
> > good idea as that could be huge information.
> > 
> > 
> > Let's handle the logging for FullUpdateInitializer.java seperate jira. 
> > Let's use the jir SENTRY-2301 for it.

Ok I'll modify it and upload a new patch


- Arjun


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


On July 20, 2018, 3:44 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 20, 2018, 3:44 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  61b0d10ca 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  992d8ab52 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  12266cb25 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[kalyan kumar kalvagadda via Review Board]

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


Fix it, then Ship it!




Please address the comment.


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
Lines 304-312 (patched)


I don't recommend this log message. 

Logging the names of databases, tables and partition names may not be good 
idea as that could be huge information.

Let's handle the logging for FullUpdateInitializer.java seperate jira. 
Let's use the jir SENTRY-2301 for it.


- kalyan kumar kalvagadda


On July 20, 2018, 3:44 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 20, 2018, 3:44 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
>  61b0d10ca 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  992d8ab52 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  12266cb25 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 20, 2018, 3:44 p.m.)


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


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 61b0d10ca 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 992d8ab52 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 12266cb25 


Diff: https://reviews.apache.org/r/67879/diff/9/

Changes: https://reviews.apache.org/r/67879/diff/8-9/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 20, 2018, 1:52 p.m.)


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


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 61b0d10ca 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 12266cb25 


Diff: https://reviews.apache.org/r/67879/diff/8/

Changes: https://reviews.apache.org/r/67879/diff/7-8/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 16, 2018, 12:07 a.m.)


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


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
 61b0d10ca 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


Diff: https://reviews.apache.org/r/67879/diff/7/

Changes: https://reviews.apache.org/r/67879/diff/6-7/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 15, 2018, 11:40 p.m.)


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


Changes
---

Newer logs + post feedback


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 13, 2018, 5:03 p.m.)


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


Changes
---

Post Kalyan's feedback


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

> On July 13, 2018, 12:37 p.m., kalyan kumar kalvagadda wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Lines 99-100 (original), 100-103 (patched)
> > 
> >
> > Along this change we need to log the reason why full update is sent to 
> > Namenode in the console. Otherwise person looking at it should go back to 
> > the logs to understand it.

You mean why full snapshot is triggered? Full update are starightforward that 
requesting image number is less than current sequence?


- Arjun


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


On July 11, 2018, 6:25 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 6:25 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[kalyan kumar kalvagadda via Review Board]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 99-100 (original), 100-103 (patched)


Along this change we need to log the reason why full update is sent to 
Namenode in the console. Otherwise person looking at it should go back to the 
logs to understand it.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Line 415 (original), 417-419 (patched)


Suggestion on the message
"Creating full snapshot is complete". 

You log says snapshot id but you are passing snapshotInfo.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Line 236 (original), 237 (patched)


Don't you think reson for triggering full update should be logged in 
console?


- kalyan kumar kalvagadda


On July 11, 2018, 6:25 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 6:25 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Na Li via Review Board]

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


Ship it!




Ship It!

- Na Li


On July 11, 2018, 6:25 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 6:25 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  719e616bf 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 11, 2018, 6:25 p.m.)


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


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 11, 2018, 6:22 p.m.)


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


Changes
---

Updated patch based on comments


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 719e616bf 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

> On July 11, 2018, 5:32 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Lines 399 (patched)
> > 
> >
> > It is better " notification Id = %d: at time %d" than " notification Id 
> > = %d: %d"

The new diff is time stamp first then message. So then we explictly don't have 
to specify that it is time


- Arjun


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


On July 11, 2018, 4:42 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 4:42 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

> On July 11, 2018, 5:56 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
> > Lines 99-100 (original), 99-102 (patched)
> > 
> >
> > Can we reuse the string instead?
> > 
> > Like, 
> >   String msg = String.format(...);
> >   LOGGER.info(msg);
> >   System.out.println(msg);
> >   
> > Same for all the other messages.

Sure I can do that


> On July 11, 2018, 5:56 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Lines 417 (patched)
> > 
> >
> > We have the date in the log, why is it needed to print it in the 
> > console?

The point of adding these console messages is so in case the logs roll over we 
can still be sure that this event has occurred and also know when they have 
occurred


> On July 11, 2018, 5:56 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
> > Lines 244 (patched)
> > 
> >
> > Why do we need to print the timestamp in the console? Can users go to 
> > the log?
> > 
> > Perhaps is good to print one single message and how much it took?

The point of adding these console messages is so in case the logs roll over we 
can still be sure that this event has occurred and also know when they have 
occurred


- Arjun


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


On July 11, 2018, 4:42 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 4:42 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Sergio Pena via Review Board]

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




sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
Lines 99-100 (original), 99-102 (patched)


Can we reuse the string instead?

Like, 
  String msg = String.format(...);
  LOGGER.info(msg);
  System.out.println(msg);
  
Same for all the other messages.



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


We have the date in the log, why is it needed to print it in the console?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 244 (patched)


Why do we need to print the timestamp in the console? Can users go to the 
log?

Perhaps is good to print one single message and how much it took?


- Sergio Pena


On July 11, 2018, 4:42 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 4:42 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Na Li via Review Board]

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




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


It is better " notification Id = %d: at time %d" than " notification Id = 
%d: %d"



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


why don't you print out the snapshot ID? It is easier to search and match 
the starting messages



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 238 (patched)


It is better to indicate the value is time in the message



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Lines 244 (patched)


same thing


- Na Li


On July 11, 2018, 4:42 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 11, 2018, 4:42 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2309
> https://issues.apache.org/jira/browse/SENTRY-2309
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 11, 2018, 4:42 p.m.)


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


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


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

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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[Arjun Mishra via Review Board]

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

(Updated July 11, 2018, 4:25 p.m.)


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


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


Repository: sentry


Description
---

Adding to std.out will allow full snapshot timeline messages like has fetch 
started, or has fetch been persisted, to be separated out from the rest of the 
logs and be identified more easily


Diffs
-

  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 065adb74f 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
 7667c47d5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 9d1a92f18 


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


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 67879: SENTRY-2309: Add relevant full snapshot timeline messages to std.out

[kalyan kumar kalvagadda via Review Board]

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



I'm not done with my review yet.


sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Line 415 (original), 416-417 (patched)


Suggestion on the message
"Creating full snapshot is complete".

To be consistent we should be logging snapshotInfo.getId() as well.


- kalyan kumar kalvagadda


On July 10, 2018, 9:54 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67879/
> ---
> 
> (Updated July 10, 2018, 9:54 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Adding to std.out will allow full snapshot timeline messages like has fetch 
> started, or has fetch been persisted, to be separated out from the rest of 
> the logs and be identified more easily
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
>  065adb74f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  7667c47d5 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  9d1a92f18 
> 
> 
> Diff: https://reviews.apache.org/r/67879/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>