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