----------------------------------------------------------- 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) <https://reviews.apache.org/r/63881/#comment268941> 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) <https://reviews.apache.org/r/63881/#comment268944> '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) <https://reviews.apache.org/r/63881/#comment268949> 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) <https://reviews.apache.org/r/63881/#comment268946> 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) <https://reviews.apache.org/r/63881/#comment268947> 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) <https://reviews.apache.org/r/63881/#comment268950> 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) <https://reviews.apache.org/r/63881/#comment268951> 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 > >
