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