You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Hao Hao <ha...@cloudera.com> on 2016/09/21 23:45:35 UTC
Review Request 52138: SENTRY-1463: Ensure HMS point-in-time snapshot
consistency
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52138/
-----------------------------------------------------------
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.
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
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time
snapshot consistency
Posted by Hao Hao <ha...@cloudera.com>.
> On Sept. 22, 2016, 12:39 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, line 212
> > <https://reviews.apache.org/r/52138/diff/1/?file=1507604#file1507604line212>
> >
> > Is the TODO: read currentEventID still relevant?
Yeah, in the case when Sentry DB already stored a full snapshot need to read from DB. Will update the comment.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52138/#review149928
-----------------------------------------------------------
On Sept. 22, 2016, 8:39 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52138/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2016, 8:39 p.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
>
>
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time
snapshot consistency
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 52138: SENTRY-1463: Ensure HMS point-in-time
snapshot consistency
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
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 (updated)
-------
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