-----------------------------------------------------------
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
> 
>

Reply via email to