You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arjun Mishra via Review Board <no...@reviews.apache.org> on 2017/12/05 17:32:56 UTC

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

-----------------------------------------------------------
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

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/63881/#comment271258>

    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

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/63881/diff/11/?file=1909888#file1909888line165>
> >
> >     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

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/63881/#comment271537>

    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

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.

> On Dec. 5, 2017, 11:25 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 420 (original), 430 (patched)
> > <https://reviews.apache.org/r/63881/diff/8/?file=1909211#file1909211line432>
> >
> >     Use `ID = {}", event.getEventId`.
> >     Also, do we want to show the actual failure here or it will be printed elsewhere?
> 
> Vadim Spector wrote:
>     For some reason processNotifications() is declared public, yet it is only used inside HMSFollower.syncupWithHms(), inside try { } catch (Throwable e) { } , which does LOG.error("Exception in HMSFollower! Caused by: " + t.getMessage(), t);
>     So, we should be fine, but I'm wondering why not to declare this method private

nevermind, it is used in JUnit tests


- Vadim


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


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

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.

> On Dec. 5, 2017, 11:25 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 377 (patched)
> > <https://reviews.apache.org/r/63881/diff/8/?file=1909211#file1909211line379>
> >
> >     Will someone up the call stack print the full stack trace?

createFullSnapshot() is private method called only from syncupWithHms() method, inside try { } catch (Throwable e) { } , which does LOG.error("Exception in HMSFollower! Caused by: " + t.getMessage(), t);
so, we should be fine as far as I can see


> On Dec. 5, 2017, 11:25 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Line 420 (original), 430 (patched)
> > <https://reviews.apache.org/r/63881/diff/8/?file=1909211#file1909211line432>
> >
> >     Use `ID = {}", event.getEventId`.
> >     Also, do we want to show the actual failure here or it will be printed elsewhere?

For some reason processNotifications() is declared public, yet it is only used inside HMSFollower.syncupWithHms(), inside try { } catch (Throwable e) { } , which does LOG.error("Exception in HMSFollower! Caused by: " + t.getMessage(), t);
So, we should be fine, but I'm wondering why not to declare this method private


> On Dec. 5, 2017, 11:25 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
> > Lines 190 (patched)
> > <https://reviews.apache.org/r/63881/diff/8/?file=1909212#file1909212line190>
> >
> >     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.

Per https://www.slf4j.org/apidocs/org/slf4j/Logger.html#info(java.lang.String,%20java.lang.Object...) it should:
Log a message at the INFO level according to the specified format and arguments.
This form avoids superfluous string concatenation when the logger is disabled for the INFO level. However, this variant incurs the hidden (and relatively small) cost of creating an Object[] before invoking the method, even if this logger is disabled for INFO. The variants taking one and two arguments exist solely in order to avoid this hidden cost.
Parameters:
format - the format string
arguments - a list of 3 or more arguments


> On Dec. 5, 2017, 11:25 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
> > Lines 209 (patched)
> > <https://reviews.apache.org/r/63881/diff/8/?file=1909212#file1909212line209>
> >
> >     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`

Agree


- Vadim


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


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

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/63881/#comment271311>

    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)
<https://reviews.apache.org/r/63881/#comment271312>

    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)
<https://reviews.apache.org/r/63881/#comment271313>

    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)
<https://reviews.apache.org/r/63881/#comment271316>

    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

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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