You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arjun Mishra via Review Board <no...@reviews.apache.org> on 2017/12/22 23:34:22 UTC
Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry
logic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
If first event in list of notifications received from HMS greater than latest sentry hms notification Id
If latest sentry notification Id is greater than current hms notification Id
The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/1/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on
HMS table
NOTIFICATION_SEQUENCE when checking if Sentry is out-of-sync with HMS
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review195749
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On Jan. 16, 2018, 3:50 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2018, 3:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> After steady state, a full snapshot is triggered by mainly 2 cases
>
> 1. Sentry is "ahead"
> 2. Sentry is behind
> Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
> The fix is to remove this dependency.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/6/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on
HMS table
NOTIFICATION_SEQUENCE when checking if Sentry is out-of-sync with HMS
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review195750
-----------------------------------------------------------
Ship it!
Ship It!
- Vadim Spector
On Jan. 16, 2018, 3:50 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2018, 3:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> After steady state, a full snapshot is triggered by mainly 2 cases
>
> 1. Sentry is "ahead"
> 2. Sentry is behind
> Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
> The fix is to remove this dependency.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/6/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on
HMS table
NOTIFICATION_SEQUENCE when checking if Sentry is out-of-sync with HMS
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 16, 2018, 3:50 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The fix is to remove this dependency.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/6/
Changes: https://reviews.apache.org/r/64820/diff/5-6/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on
HMS table
NOTIFICATION_SEQUENCE when checking if Sentry is out-of-sync with HMS
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 16, 2018, 3:50 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Summary (updated)
-----------------
SENTRY-2106: Remove sentry dependency on HMS table NOTIFICATION_SEQUENCE when checking if Sentry is out-of-sync with HMS
Repository: sentry
Description
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The fix is to remove this dependency.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/5/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on
HMS table NOTIFICATION_SEQUENCE
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 7:23 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The fix is to remove this dependency.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/5/
Changes: https://reviews.apache.org/r/64820/diff/4-5/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on
HMS table NOTIFICATION_SEQUENCE
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 7:21 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Summary (updated)
-----------------
SENTRY-2106: Remove sentry dependency on HMS table NOTIFICATION_SEQUENCE
Repository: sentry
Description (updated)
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The fix is to remove this dependency.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/4/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Eliminate sentry ahead logic
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194770
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 263 (original), 263 (patched)
<https://reviews.apache.org/r/64820/#comment273747>
should we get rid of this call for .getCurrentNotificationId?
- Na Li
On Jan. 4, 2018, 5:06 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2018, 5:06 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> After steady state, a full snapshot is triggered by mainly 2 cases
>
> 1. Sentry is "ahead"
> 2. Sentry is behind
> Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
> The solution is to eliminate this check. Sentry can never be ahead of HMS.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/4/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Eliminate sentry ahead logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 5:06 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Summary (updated)
-----------------
SENTRY-2106: Eliminate sentry ahead logic
Repository: sentry
Description
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The solution is to eliminate this check. Sentry can never be ahead of HMS.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/4/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 5:05 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Changes
-------
Removing the sentry "ahead" logic
Repository: sentry
Description
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The solution is to eliminate this check. Sentry can never be ahead of HMS.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/4/
Changes: https://reviews.apache.org/r/64820/diff/3-4/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 4:43 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description (updated)
-------
After steady state, a full snapshot is triggered by mainly 2 cases
1. Sentry is "ahead"
2. Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The solution is to eliminate this check. Sentry can never be ahead of HMS.
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/3/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 4:40 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description (updated)
-------
After steady state, a full snapshot is triggered by mainly 2 cases
Sentry is "ahead"
Sentry is behind
Case 1 has a dependency on NOTIFICATION_SEQUENCE table. This is not reliable as it was observed that sometimes NOTIFICATION_SEQUENCE and NOTIFICATION_LOG are not in sync. As a result of this unnecessary full snapshots can be triggered.
The solution is to eliminate this check. Sentry can never be ahead of HMS.
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/3/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194500
-----------------------------------------------------------
Ship it!
Ship It!
- Vadim Spector
On Dec. 25, 2017, 10:49 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2017, 10:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/3/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On Dec. 28, 2017, 5:21 p.m., kalyan kumar kalvagadda wrote:
> > I have couple of comments on the approach taken. Before going into that, I would re-iterate couple of assumptions that HMSFollower has taken
> >
> > 1. Method getCurrentNotificationEventId on Hmsclient would always return the last issued notification event id, which HMSFollower assumes to be the biggest. Which doesn't seems to be true. We don't have any conclusive answers on this behavior of HMS.
> > 2. First event in list of notifications received from HMS greater than latest sentry HMS notification Id. This is not true
> > Event_ID in the NOTIFICATION_LOG table are not always ascending. Order of sequence is not guaranteed.
> > There can be gaps in between Event_ID's in the consecutive entries.
> > Here is what I think.
> >
> > when getCurrentNotificationEventId is not dependable why should HMSFollower use it in the first place.
> > Even with the fix that you provided HMSFollower would wait for 5 sec(Default) to see if getCurrentNotificationEventId returns a value greater than the last event id that sentry has processed. If not, full snapshot is initiated. There is no guaranty that it would be greater by that time. Sentry is not out of sync even if getCurrentNotificationEventId returns an Id that is smaller than the last event id that sentry has processed. Do you agree?
> >
> > I prefer removing code in HMSFollower that depends on getCurrentNotificationEventId to see if it out of sync with HMS.
> > Fix for SENTRY-2109 will address the 2nd assumption taken.
>
> Arjun Mishra wrote:
> I agree about removing dependency on getCurrentNotificationEventId, but I don't see another way to test for cases where Sentry is "ahead". Unless you want to remove the case to trigger a full snapshot if Sentry is "ahead"?
>
> The motivation for this patch was to KEEP THE EXISTING logic but make it more restrictive. The fix should have an improvement because in 5 sec, or 10 retires, Sentry should have been "ahead" on EVERY retry for a Full Snapshot to occur.
>
> But if all agree that Sentry being "ahead" should not be a criteria to trigger a ful snapshot we can remove it.
Firstly, Sentry can never be ahead of HMS. That's not a possibility under normal circumstances. I think, we can remove that condition which triggers full snapshot.
HMSFollower is thinking that it is, because of it's assumtion that value returned by getCurrentNotificationEventId is always greater than the MAX(event-id) that sentry processed.
This situation is possible only when HMS data is reset and all the data is reset. we need to understad the possibilities of this. When some thing like this happens, administrator should request for snapshot explicitly. There is seperate effort being put on this.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194585
-----------------------------------------------------------
On Dec. 25, 2017, 10:49 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2017, 10:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/3/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Dec. 28, 2017, 5:21 p.m., kalyan kumar kalvagadda wrote:
> > I have couple of comments on the approach taken. Before going into that, I would re-iterate couple of assumptions that HMSFollower has taken
> >
> > 1. Method getCurrentNotificationEventId on Hmsclient would always return the last issued notification event id, which HMSFollower assumes to be the biggest. Which doesn't seems to be true. We don't have any conclusive answers on this behavior of HMS.
> > 2. First event in list of notifications received from HMS greater than latest sentry HMS notification Id. This is not true
> > Event_ID in the NOTIFICATION_LOG table are not always ascending. Order of sequence is not guaranteed.
> > There can be gaps in between Event_ID's in the consecutive entries.
> > Here is what I think.
> >
> > when getCurrentNotificationEventId is not dependable why should HMSFollower use it in the first place.
> > Even with the fix that you provided HMSFollower would wait for 5 sec(Default) to see if getCurrentNotificationEventId returns a value greater than the last event id that sentry has processed. If not, full snapshot is initiated. There is no guaranty that it would be greater by that time. Sentry is not out of sync even if getCurrentNotificationEventId returns an Id that is smaller than the last event id that sentry has processed. Do you agree?
> >
> > I prefer removing code in HMSFollower that depends on getCurrentNotificationEventId to see if it out of sync with HMS.
> > Fix for SENTRY-2109 will address the 2nd assumption taken.
I agree about removing dependency on getCurrentNotificationEventId, but I don't see another way to test for cases where Sentry is "ahead". Unless you want to remove the case to trigger a full snapshot if Sentry is "ahead"?
The motivation for this patch was to KEEP THE EXISTING logic but make it more restrictive. The fix should have an improvement because in 5 sec, or 10 retires, Sentry should have been "ahead" on EVERY retry for a Full Snapshot to occur.
But if all agree that Sentry being "ahead" should not be a criteria to trigger a ful snapshot we can remove it.
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194585
-----------------------------------------------------------
On Dec. 25, 2017, 10:49 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2017, 10:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/3/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194585
-----------------------------------------------------------
I have couple of comments on the approach taken. Before going into that, I would re-iterate couple of assumptions that HMSFollower has taken
1. Method getCurrentNotificationEventId on Hmsclient would always return the last issued notification event id, which HMSFollower assumes to be the biggest. Which doesn't seems to be true. We don't have any conclusive answers on this behavior of HMS.
2. First event in list of notifications received from HMS greater than latest sentry HMS notification Id. This is not true
Event_ID in the NOTIFICATION_LOG table are not always ascending. Order of sequence is not guaranteed.
There can be gaps in between Event_ID's in the consecutive entries.
Here is what I think.
when getCurrentNotificationEventId is not dependable why should HMSFollower use it in the first place.
Even with the fix that you provided HMSFollower would wait for 5 sec(Default) to see if getCurrentNotificationEventId returns a value greater than the last event id that sentry has processed. If not, full snapshot is initiated. There is no guaranty that it would be greater by that time. Sentry is not out of sync even if getCurrentNotificationEventId returns an Id that is smaller than the last event id that sentry has processed. Do you agree?
I prefer removing code in HMSFollower that depends on getCurrentNotificationEventId to see if it out of sync with HMS.
Fix for SENTRY-2109 will address the 2nd assumption taken.
- kalyan kumar kalvagadda
On Dec. 25, 2017, 10:49 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2017, 10:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/3/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Dec. 25, 2017, 10:49 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
If first event in list of notifications received from HMS greater than latest sentry hms notification Id
If latest sentry notification Id is greater than current hms notification Id
The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/3/
Testing (updated)
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Dec. 25, 2017, 10:48 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Changes
-------
Post Vadim's suggestions
Repository: sentry
Description
-------
Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
If first event in list of notifications received from HMS greater than latest sentry hms notification Id
If latest sentry notification Id is greater than current hms notification Id
The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/3/
Changes: https://reviews.apache.org/r/64820/diff/2-3/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194479
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 239 (original), 253 (patched)
<https://reviews.apache.org/r/64820/#comment273254>
Add another item: "The current notification Id on the HMS is less than the latest processed by Sentry after the configurable number of retries, as detected by isSentryAhead() method"
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 329 (patched)
<https://reviews.apache.org/r/64820/#comment273255>
This "else" clause is also engaged as part of the normal flow. Because calling isSentryAhead() method is part of the normal flow and normally "if (latestSentryNotificationId > currentHmsNotificationId)" would be FALSE, so this "else" clause would be engaged.
Therefore, we should print this message only if we know we are in the state of trying to recover from "Sentry-ahead" situation. Which is when notificationSyncRetryCount != 0.
Therefore, this log should be inside the "if (notificationSyncRetryCount != 0)" clause, it seems.
- Vadim Spector
On Dec. 24, 2017, 1:50 a.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2017, 1:50 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/2/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194489
-----------------------------------------------------------
Seems like I have no problems with the code once my comments in the previous review are addressed. Would like to add more checks to the tests, including, say, a couple of loop iterations for the state after "sentry-ahead" state has been cleared, just to make sure HMSFollower is in the normal flow mode (no Sentry-ahead condition, gets partial updates normally, does not attempt full updates for no good reason, hmsFollower.getNotificationSyncRetryCount() keeps returning 0).
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 263 (original), 275 (patched)
<https://reviews.apache.org/r/64820/#comment273272>
typo "cuurent"
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 1108 (patched)
<https://reviews.apache.org/r/64820/#comment273275>
Would it make sense to add verify() on sentryHmsClient.getFullSnapshot() invokactions? Can't hurt it seems
Also, would it make sense to continue the loop for sometime after count == interruptCount instead of break? To check the normal flow after "sentry-ahead" recovery? And add default "else" clause with verify()'s for all mock objects? Post-sentry-ahead state is an important part to test too.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java
Lines 1179 (patched)
<https://reviews.apache.org/r/64820/#comment273274>
Would it make sense to add verify() on sentryHmsClient.getFullSnapshot() invocations? Can't hurt it seems.
Also, would it make sense to continue the loop for sometime after count == interruptCount instead of break? To check the normal flow after "sentry-ahead" recovery? And add default "else" clause with verify()'s for all mock objects? Post-sentry-ahead state is an important part to test too.
- Vadim Spector
On Dec. 24, 2017, 1:50 a.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2017, 1:50 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/2/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/
-----------------------------------------------------------
(Updated Dec. 24, 2017, 1:50 a.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Changes
-------
Updates post Vadim's feedback
Repository: sentry
Description
-------
Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
If first event in list of notifications received from HMS greater than latest sentry hms notification Id
If latest sentry notification Id is greater than current hms notification Id
The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
Diff: https://reviews.apache.org/r/64820/diff/2/
Changes: https://reviews.apache.org/r/64820/diff/1-2/
Testing
-------
mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
Thanks,
Arjun Mishra
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194472
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 71 (patched)
<https://reviews.apache.org/r/64820/#comment273240>
A couple of naming suggestions:
a) fullUdateRetryCount is a bit misleading; count is something that's incremented, not a constant. fullUpdateAttemptMax would be more intuitive, makes it clearly related to fullUpdateAttemptCount. Or you can use "Retry" in both names - fullUpdateRetryCount and fullUpdateRetryMax.
b) the names are probably misleading altogether; it is not an attempt to do full update, but on contrary an attempt to avoid it; essentially, an attempt to establish that Sentry and HMS are in-sync, so no full update is needed. I'd change it to something like notificationSyncRetryCount and notificationSyncRetryMax
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 228 (patched)
<https://reviews.apache.org/r/64820/#comment273239>
nit: extra line
- Vadim Spector
On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2017, 11:34 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194475
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 273 (original), 284 (patched)
<https://reviews.apache.org/r/64820/#comment273242>
because you invoke isSentryAhead() outside this method, the fullUpdateHMS.compareAndSet(true, false) code is not invoked when Sentry-is-ahead state is detected. Another reason to call isSentryAhead() from this method.
- Vadim Spector
On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2017, 11:34 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194476
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 542 (patched)
<https://reviews.apache.org/r/64820/#comment273243>
this method is used from TestHMSFollower, which is the same package, so it can be declared package-private instead of public
- Vadim Spector
On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2017, 11:34 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot
retry logic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64820/#review194474
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 219 (patched)
<https://reviews.apache.org/r/64820/#comment273241>
isSentryAhead() is part of the logic to decide if full snapshot is required, so it would make sense to invoke it inside the isFullSnapshotRequired(), instead of adding this block of code here. After all, isSentryAhead() is an improved version of the code that used to be exactly in isFullSnapshotRequired(), which you've removed.
And the javadoc section for isFullSnapshotRequired() that you removed would stay, just with some minor modifications. It will result in fewer diffs and will be easier to undrestand what's exactly changed.
- Vadim Spector
On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64820/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2017, 11:34 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Once SentryHMSNotification table is not empty, there are two cases when a full snapshot is triggered.
>
> If first event in list of notifications received from HMS greater than latest sentry hms notification Id
> If latest sentry notification Id is greater than current hms notification Id
> The later is a unexpected case where for some reason sentry gets ahead of HMS. We should add a logic to trigger a full snapshot in case 2 only after a configurable number of retries. This will avoid unnecessary full snapshot triggers as they are resource intensive
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a31c
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886a7
>
>
> Diff: https://reviews.apache.org/r/64820/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower
>
>
> Thanks,
>
> Arjun Mishra
>
>