----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62221/#review185309 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 53 (patched) <https://reviews.apache.org/r/62221/#comment261611> Hmm, isn't this what methods are for? Why not have public String retrieverType() method? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 74 (original), 79 (patched) <https://reviews.apache.org/r/62221/#comment261613> WHy do you want to combine String format with Logger formatting? You may just use String format to get all three. And the log can be simply `Get all %s updates for image %d from sequence %d` sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 79 (original), 84 (patched) <https://reviews.apache.org/r/62221/#comment261615> Suggested format here and other places is `(PATH) something something` Why not `Current PATH image ...` sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 95 (patched) <https://reviews.apache.org/r/62221/#comment261614> can it be null? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Lines 143 (patched) <https://reviews.apache.org/r/62221/#comment261620> If you are going to the trouble of logging it, please also log pathSeqNum and pathImgNum sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 201 (original), 203 (patched) <https://reviews.apache.org/r/62221/#comment261619> We go to so much trouble to print this message and I don't see why it is useful. The important part is the advance of notification IDs that can be tracked through metrics. What if it successfully processed one message but not the rest? How would this message help someone? - Alexander Kolbasov On Sept. 13, 2017, 2:26 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62221/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2017, 2:26 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Na Li, Sergio > Pena, and Vamsee Yarlagadda. > > > Repository: sentry > > > Description > ------- > > Sentry Supportability improvement. > > 1. HMSFollower > * Print confirmation message (at INFO level) once full snapshot is persisted > in the DB. > * Print the message that HMSFollower is completely ready (after the initial > pass of HMSFollower is done) > > 2. DBUpdateForwarder > * Every log message should explicitly mention which type of events is it > referring to (PERM or PATH) otherwise there is no way for us to differentiate > between calls. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > 8a34d5623 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 1318082d3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 31fd4597d > > > Diff: https://reviews.apache.org/r/62221/diff/5/ > > > Testing > ------- > > > Thanks, > > Arjun Mishra > >
