Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-08 Thread Arjun Mishra via Review Board 


> On Dec. 6, 2017, 9:12 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
> > Line 164 (original), 165 (patched)
> > 
> >
> > Question for this and similar parts of code: will it be common for old 
> > and new values to have gaps, and if not, would it make sense to warn() 
> > about it?

Vadim, as discussed yesterday since a full snapshot is resource intensive and 
takes several minutes, it is possible that newValue > oldValue + 1. With this 
specific case we don't need to log a warning message


- Arjun


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


On Dec. 6, 2017, 9:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Dec. 6, 2017, 9:03 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
>  2268ce740 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/11/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-06 Thread Vadim Spector via Review Board 

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
Line 164 (original), 165 (patched)


Question for this and similar parts of code: will it be common for old and 
new values to have gaps, and if not, would it make sense to warn() about it?


- Vadim Spector


On Dec. 6, 2017, 9:03 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Dec. 6, 2017, 9:03 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
>  2268ce740 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/11/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-06 Thread Arjun Mishra via Review Board 

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

(Updated Dec. 6, 2017, 9:03 p.m.)


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


Changes
---

Added new CounterWait logs


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/CounterWait.java
 2268ce740 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


Diff: https://reviews.apache.org/r/63881/diff/11/

Changes: https://reviews.apache.org/r/63881/diff/10-11/


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-06 Thread Arjun Mishra via Review Board 

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

(Updated Dec. 6, 2017, 8:25 p.m.)


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


Changes
---

Added logging to wakeUpWaitingClientsForSync() method


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-06 Thread Arjun Mishra via Review Board 

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

(Updated Dec. 6, 2017, 4:01 p.m.)


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


Changes
---

Updating Patch based on Sasha's feedback


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Alexander Kolbasov 

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




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


Will someone up the call stack print the full stack trace?



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


Use `ID = {}", event.getEventId`.
Also, do we want to show the actual failure here or it will be printed 
elsewhere?



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


Does this work? I am not sure that logger correctly supports more then two 
args. You can pass Array with more then 2 elements though.



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


It is visuall better to rearrange these to show the lower ID first.

It would be nice to see something like `received non-consequitive event 
sequence 10, 12`


- Alexander Kolbasov


On Dec. 5, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Dec. 5, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/8/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Vadim Spector via Review Board 

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


Ship it!




Ship It!

- Vadim Spector


On Dec. 5, 2017, 9:02 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Dec. 5, 2017, 9:02 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/8/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Arjun Mishra via Review Board 

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

(Updated Dec. 5, 2017, 9:02 p.m.)


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


Changes
---

Included ExceptionUtils


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Vadim Spector via Review Board 

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




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


I recommend using 
org.apache.commons.lang3.exception.ExceptionUtils.getRootCause() instead, which 
is available. See 
https://commons.apache.org/proper/commons-lang/apidocs/src-html/org/apache/commons/lang3/exception/ExceptionUtils.html

It addresses weird case where exception chain forms a  linked list with the 
loop, which would result in infinite loop. In theory shoud never happen, but 
not impossible.


- Vadim Spector


On Dec. 5, 2017, 5:32 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Dec. 5, 2017, 5:32 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/7/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Arjun Mishra via Review Board 

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

(Updated Dec. 5, 2017, 5:32 p.m.)


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


Changes
---

Taking into account Vadim's suggestions and new updates


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-30 Thread Arjun Mishra via Review Board 


> On Nov. 30, 2017, 11:07 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 346 (patched)
> > 
> >
> > Is getting empty snapshot while creating full snapshot normal? If not, 
> > perhaps info() or warn() would be better? Also, would not hurt including 
> > snapshotInfo.getId() into the log.

So it is usually the first case to hit, or the first cause of a full snap shot. 
So yes the chances of hitting this is high. The reason why it is debug is, as 
Sergio suggested, that if Sentry service is started, and no "work" is being 
done on HMS, we will always trigger creating a full snapshot. Only snapshot 
will be empty, so we will continue to trigger it. In which case this would be 
logged every 0.5 sec


- Arjun


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


On Nov. 30, 2017, 10:41 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 30, 2017, 10:41 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-30 Thread Vadim Spector via Review Board 

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




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


Is getting empty snapshot while creating full snapshot normal? If not, 
perhaps info() or warn() would be better? Also, would not hurt including 
snapshotInfo.getId() into the log.



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


logging eventIdBefore may be useful at this point



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
Line 202 (original), 203 (patched)


Would it be worth logging normal execution flow inside this loop? It is 
still exceptional in a way that it's inside full update logic, right? Should 
not be too many iterations before currentEventId and eventIdAfter catch up with 
each other, is it right? Something like "{} updates received, currentEventId 
{}, eventIdAfter {}".

Also, in theory returned event IDs should be sequential, but this is HMS, 
so who knows what can happen? The logic breaks out of the loop if the event ID 
reaches or exceeds the desired value, but if sequence numbers are out of order 
we can get gaps with hard to debug HDFS sync problems. The least we can do is 
warn() about sequence numbers being non-sequential, something like

if (event.getEventId() != (currentEventId+1)) {
  LOGGER.warn(...);
}


- Vadim Spector


On Nov. 30, 2017, 10:41 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 30, 2017, 10:41 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-30 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 30, 2017, 10:41 p.m.)


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


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-30 Thread Sergio Pena via Review Board 

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


Ship it!




Ship It!

- Sergio Pena


On Nov. 29, 2017, 5:43 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 29, 2017, 5:43 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/5/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-30 Thread Sergio Pena via Review Board 


> On Nov. 29, 2017, 4:50 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 251-252 (patched)
> > 
> >
> > This line is making me some noise. If a user upgrades  their cluster, 
> > then the MSentryHmsNotification table will be empty initially; and if the 
> > user does not execute any DDL command for a long time, then this message 
> > will appear every 500ms on the log.
> > 
> > Is this log message necessary here?
> 
> Arjun Mishra wrote:
> Yeah it can be debug. I'll change it
> 
> Na Li wrote:
> Sergio, in this case, sentry will keep on getting full snapshot when 
> upser upgrade their cluster, and not execute any DDL command for a long time. 
> It is not desirable. We should re-visit this logic on when to get full 
> snapshot.

Agree. I think we should write a snapshot even if it is empty.

This below code is the one returning if an attempt to create a snapshot doesn't 
do anything. We may need to persist the snapshot ID at least if it is empty.

private long createFullSnapshot() {
PathsImage snapshotInfo = client.getFullSnapshot();
if (snapshotInfo.getPathImage().isEmpty()) {
   return snapshotInfo.getId();
}
}


- Sergio


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


On Nov. 29, 2017, 5:43 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 29, 2017, 5:43 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/5/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Na Li via Review Board 


> On Nov. 29, 2017, 4:50 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 251-252 (patched)
> > 
> >
> > This line is making me some noise. If a user upgrades  their cluster, 
> > then the MSentryHmsNotification table will be empty initially; and if the 
> > user does not execute any DDL command for a long time, then this message 
> > will appear every 500ms on the log.
> > 
> > Is this log message necessary here?
> 
> Arjun Mishra wrote:
> Yeah it can be debug. I'll change it

Sergio, in this case, sentry will keep on getting full snapshot when upser 
upgrade their cluster, and not execute any DDL command for a long time. It is 
not desirable. We should re-visit this logic on when to get full snapshot.


- Na


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


On Nov. 29, 2017, 5:43 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 29, 2017, 5:43 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/5/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 29, 2017, 5:43 p.m.)


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


Changes
---

Updating patch based on Sasha's feedback


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Alexander Kolbasov 

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




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


Note that you are loggint this before transaction committed, (or even 
before you write actual snapshot) so it may be possible that you'll see the log 
that snapshot is created but it would not be actually created due to some issue 
with transaction commit.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2912 (original), 2913 (patched)


It would be also useful to log currentSnapshotID. Also, this is probably 
warning, not error. Do you want to log the full authzObject here or jost some 
relevant info from it?



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


Message can be quite big - do we really want to log it?


- Alexander Kolbasov


On Nov. 29, 2017, 5:28 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 29, 2017, 5:28 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/4/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 29, 2017, 5:28 p.m.)


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


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 29, 2017, 5:24 p.m.)


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


Changes
---

Update based on Sergio's feedback


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Arjun Mishra via Review Board 


> On Nov. 29, 2017, 4:50 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 251-252 (patched)
> > 
> >
> > This line is making me some noise. If a user upgrades  their cluster, 
> > then the MSentryHmsNotification table will be empty initially; and if the 
> > user does not execute any DDL command for a long time, then this message 
> > will appear every 500ms on the log.
> > 
> > Is this log message necessary here?

Yeah it can be debug. I'll change it


- Arjun


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


On Nov. 27, 2017, 4:39 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 27, 2017, 4:39 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-29 Thread Sergio Pena via Review Board 

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




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


This line is making me some noise. If a user upgrades  their cluster, then 
the MSentryHmsNotification table will be empty initially; and if the user does 
not execute any DDL command for a long time, then this message will appear 
every 500ms on the log.

Is this log message necessary here?


- Sergio Pena


On Nov. 27, 2017, 4:39 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 27, 2017, 4:39 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-27 Thread Arjun Mishra via Review Board 


> On Nov. 27, 2017, 6:22 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 351 (patched)
> > 
> >
> > this should be debug level or log once only every hour. Otherwise, the 
> > log could be flooded by this message.

isLeader() method is called in HMSFollower run() method as well. So if one of 
the services is not a leader we ideally should never call syncupWithHms() 
method. The only time this can happen is at the time we called the 
syncupWithHms() method it was a leader, but by the time we called 
createFullSnapshot() method it was not a leader. This seems like a rare case. 
That's why I don't think it should flood the logs.


> On Nov. 27, 2017, 6:22 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 360 (patched)
> > 
> >
> > this should be debug level to avoid too many messages.

This should happen only when createFullSnapShot is being triggered, 
snapshot.getPathImage() is not empty, and HDFSSync is disabled. It shouldn't 
occur that often in my opinion. We only call createFullSnapshot if sentry HMS 
notification table is empty, or sentry-hms are out of sync. Can you explain in 
what scenario this will flood the log?


- Arjun


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


On Nov. 27, 2017, 4:39 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 27, 2017, 4:39 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  f32a745ed 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c1471d118 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  eccb40fb6 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/2/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-27 Thread Arjun Mishra via Review Board 

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

(Updated Nov. 27, 2017, 4:39 p.m.)


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


Changes
---

Accounting for all the suggestions


Repository: sentry


Description
---

Currently we only seem to log when all conditions that lead to getting a 
snapshot are validated, and eventually a full snapshot is received. However, if 
for some reason a particular condition was invalid we don't log as to why. This 
leaves no room for knowing definitely what might have been the reason as to why 
a snapshot was not received from HMS.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 f32a745ed 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
 c1471d118 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 eccb40fb6 


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

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


Testing
---

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


Thanks,

Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-16 Thread Sergio Pena via Review Board 

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




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


Should we move this after persisting the snapshot? I think is better to 
know what the new snapshot is once is persisted and not before as errors could 
happen ans the id won't never be persisted.

Also, let's have cleared info message. How would users know what 'next 
snapshot id 5' means? is a snapshot of hdfs? Perhaps something like 'New HMS 
snapshot with ID = {} is been created"



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2912 (original), 2913 (patched)


'an paths' is not correct. 'an' is used on singular nouns. It should be 'a'



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


Do we need to log the notification ID here? The log for 'has no hms 
notifications' is enough, isn't it?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 254-255 (original), 256-257 (patched)


Could you put the {} in parenthesis, like (Id = {}). That will let us know 
what the number means.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 307-308 (original), 309-310 (patched)


Could you put the {} in parenthesis, like (Id = {}). That will let us know 
what the number means.



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


This line is going to be too verbose if we enable debug. The HMSFollower 
runs every 500ms and this would be logged every 500ms too.



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


This can be too verbose. If there are new notifications every 5s for 
instance, and we don't care about thos, then this will be printed every 5s. Can 
we do something better here that avoids such amount of verbose logs?


- Sergio Pena


On Nov. 16, 2017, 7:30 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 16, 2017, 7:30 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7217dea7a 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  6ec163b1d 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-16 Thread Na Li via Review Board 


> On Nov. 16, 2017, 7:41 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 348 (original), 353 (patched)
> > 
> >
> > You can add a log indicating we don't persist the full snapshot.
> 
> Arjun Mishra wrote:
> I thought of that. But it is already handled. If you were to look at 
> setLastProcessedNotificationID method, the first statement is a log statement 
> like below.
> 
> LOGGER.debug("Persisting Last Processed Notification ID {}", 
> notificationId);
> 
> 
> And since this method is being called only in one place I thought it 
> would be redundant.

You need to add info level message similar like "full snapshot is not persisted 
because HDFS sync is disabled". LOGGER.debug("Persisting Last Processed 
Notification ID {}", notificationId); does not contain this information.


- Na


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


On Nov. 16, 2017, 7:30 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 16, 2017, 7:30 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7217dea7a 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  6ec163b1d 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-16 Thread Arjun Mishra via Review Board 


> On Nov. 16, 2017, 7:41 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 348 (original), 353 (patched)
> > 
> >
> > You can add a log indicating we don't persist the full snapshot.

I thought of that. But it is already handled. If you were to look at 
setLastProcessedNotificationID method, the first statement is a log statement 
like below.

LOGGER.debug("Persisting Last Processed Notification ID {}", notificationId);


And since this method is being called only in one place I thought it would be 
redundant.


- Arjun


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


On Nov. 16, 2017, 7:30 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 16, 2017, 7:30 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7217dea7a 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  6ec163b1d 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-16 Thread Na Li via Review Board 

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




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


You can add a log indicating we don't persist the full snapshot.


- Na Li


On Nov. 16, 2017, 7:30 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63881/
> ---
> 
> (Updated Nov. 16, 2017, 7:30 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and 
> Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently we only seem to log when all conditions that lead to getting a 
> snapshot are validated, and eventually a full snapshot is received. However, 
> if for some reason a particular condition was invalid we don't log as to why. 
> This leaves no room for knowing definitely what might have been the reason as 
> to why a snapshot was not received from HMS.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  7217dea7a 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
>  c4cc91806 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  6ec163b1d 
> 
> 
> Diff: https://reviews.apache.org/r/63881/diff/1/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>