----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review150197 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 75 - 77) <https://reviews.apache.org/r/52138/#comment218062> point-in-time. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 93 - 95) <https://reviews.apache.org/r/52138/#comment218066> Why do you need fullUpdateConmplete variable at all and why is it atomic? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 215 - 216) <https://reviews.apache.org/r/52138/#comment218067> This TODO seems obsolete - isn't what you are doing in the code below? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 217 - 237) <https://reviews.apache.org/r/52138/#comment218070> Please add a comment describing state of affairs when the code doesn't converge. What happens in this case? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 217 - 237) <https://reviews.apache.org/r/52138/#comment218072> I think for HMS it is called Notification ID, so it is better to use NotificationID name in the log to be consistent with HMS terminology sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 239 - 257) <https://reviews.apache.org/r/52138/#comment218074> Are there any established markers in the log for such events? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 239 - 257) <https://reviews.apache.org/r/52138/#comment218075> Please also add the value of event ID here - it may be useful to debug from Hive side. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 239 - 257) <https://reviews.apache.org/r/52138/#comment218078> Same comments for failure to get full update sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 239 - 257) <https://reviews.apache.org/r/52138/#comment218079> should it be an exception rather than return? Or return false to the caller to let it know that update fails. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 239 - 257) <https://reviews.apache.org/r/52138/#comment218080> It seems that the whole fullUpdateCOmplete isn't used. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (lines 239 - 257) <https://reviews.apache.org/r/52138/#comment218082> Please put a message in the log telling at which ID the full snapshot was taken sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (lines 130 - 134) <https://reviews.apache.org/r/52138/#comment218084> Please add a comment explaining what happens when the max is reached. - Alexander Kolbasov On Sept. 23, 2016, 6:15 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2016, 6:15 a.m.) > > > Review request for sentry, Alexander Kolbasov, Anne Yu, Li Li, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > SENTRY-1463: Ensure HMS point-in-time snapshot consistency > > The implemented logic is: > 1. Read current HMS notification ID_initial > 2. Read HMS metadata state > 3. Read current notification ID_new > 4. If ID_initial != ID_new then discard the current state and goto 1. > > Use configurable property: sentry.hms.snapshot.retries.max.count for max > number of retry. > > Change-Id: I7590076b875bd97b2fb340008926ea5995896d72 > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 894fcc966b511ccf309599fd10960f9a11ae8e96 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > abc3f58d21bb774427a34399b6e9f51a37ba51db > > Diff: https://reviews.apache.org/r/52138/diff/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >
