----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52138/#review149928 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 75) <https://reviews.apache.org/r/52138/#comment217697> Please add comment explaining what this variable controls. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 212) <https://reviews.apache.org/r/52138/#comment217699> Is the TODO: read currentEventID still relevant? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 214) <https://reviews.apache.org/r/52138/#comment217702> You have a good description of the logic in the commit comment - can you put it in coments here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 216) <https://reviews.apache.org/r/52138/#comment217700> Actually client pointer can be null here. This is a problem with prior code, can you fix it as well? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 219) <https://reviews.apache.org/r/52138/#comment217701> Should we retry or give up if fetchFullUpdate() fails? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 220) <https://reviews.apache.org/r/52138/#comment217703> Add TODO item to expose the retry counter in the metrics. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 221) <https://reviews.apache.org/r/52138/#comment217704> Please log a message showing the original notification ID and the new notification ID. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 223) <https://reviews.apache.org/r/52138/#comment217705> What happens if you didn't converge? Yu should fail in this case (and please log a message as well). sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 241) <https://reviews.apache.org/r/52138/#comment217706> Actual fetchFullUpdate() doesn't require coalesced HMS, the caller does, so the comment should be updated. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 130) <https://reviews.apache.org/r/52138/#comment217707> Please add comment explaining what this variable means. - Alexander Kolbasov On Sept. 21, 2016, 11:47 p.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52138/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2016, 11:47 p.m.) > > > Review request for sentry 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 > >
