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 2018/07/23 21:49:56 UTC
Review Request 68025: SENTRY-2324: Allow sentry to fetch configurable
notifications from HMS
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68025/
-----------------------------------------------------------
Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
Repository: sentry
Description
-------
Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 7667c47d5
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
Diff: https://reviews.apache.org/r/68025/diff/1/
Testing
-------
$ mvn -f sentry-service/sentry-service-server/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from HMS
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On July 24, 2018, 5:34 p.m., kalyan kumar kalvagadda wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
> > Lines 54 (patched)
> > <https://reviews.apache.org/r/68025/diff/1/?file=2062927#file2062927line54>
> >
> > There is apace between "-" and "1". Please correct it. Why not consider o as the default value?
I was thinking we could 0 as legitimate fetch value when Sentry doesn't want to fetch anything
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68025/#review206399
-----------------------------------------------------------
On July 23, 2018, 9:50 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68025/
> -----------------------------------------------------------
>
> (Updated July 23, 2018, 9:50 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
>
>
> Bugs: SENTRY-2324
> https://issues.apache.org/jira/browse/SENTRY-2324
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 7667c47d5
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
>
>
> Diff: https://reviews.apache.org/r/68025/diff/1/
>
>
> Testing
> -------
>
> $ mvn -f sentry-service/sentry-service-server/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from HMS
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On July 24, 2018, 5:34 p.m., kalyan kumar kalvagadda wrote:
> > Please add a unit test in TestHMSFollower to get full test coverage of your change.
I was considering adding unit test to TestHMSFollower but it was all Mock objects where you set what output you want. So it wasn't achieving anything. Let me know if you disagree
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68025/#review206399
-----------------------------------------------------------
On July 23, 2018, 9:50 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68025/
> -----------------------------------------------------------
>
> (Updated July 23, 2018, 9:50 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
>
>
> Bugs: SENTRY-2324
> https://issues.apache.org/jira/browse/SENTRY-2324
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 7667c47d5
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
>
>
> Diff: https://reviews.apache.org/r/68025/diff/1/
>
>
> Testing
> -------
>
> $ mvn -f sentry-service/sentry-service-server/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from HMS
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/68025/#review206399
-----------------------------------------------------------
Please add a unit test in TestHMSFollower to get full test coverage of your change.
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
Lines 54 (patched)
<https://reviews.apache.org/r/68025/#comment289336>
There is apace between "-" and "1". Please correct it. Why not consider o as the default value?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
Lines 74 (patched)
<https://reviews.apache.org/r/68025/#comment289337>
You useed fetch_size and also used fetch_depth. Please be in-line by changing one of them.
- kalyan kumar kalvagadda
On July 23, 2018, 9:50 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68025/
> -----------------------------------------------------------
>
> (Updated July 23, 2018, 9:50 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
>
>
> Bugs: SENTRY-2324
> https://issues.apache.org/jira/browse/SENTRY-2324
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 7667c47d5
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
>
>
> Diff: https://reviews.apache.org/r/68025/diff/1/
>
>
> Testing
> -------
>
> $ mvn -f sentry-service/sentry-service-server/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from HMS
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/68025/#review206805
-----------------------------------------------------------
Ship it!
Ship It!
- kalyan kumar kalvagadda
On July 26, 2018, 3:03 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68025/
> -----------------------------------------------------------
>
> (Updated July 26, 2018, 3:03 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
>
>
> Bugs: SENTRY-2324
> https://issues.apache.org/jira/browse/SENTRY-2324
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java fb826cfa6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java 1b4fa47dc
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
>
>
> Diff: https://reviews.apache.org/r/68025/diff/2/
>
>
> Testing
> -------
>
> $ mvn -f sentry-service/sentry-service-server/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from 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/68025/#review206505
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On July 26, 2018, 3:03 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68025/
> -----------------------------------------------------------
>
> (Updated July 26, 2018, 3:03 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
>
>
> Bugs: SENTRY-2324
> https://issues.apache.org/jira/browse/SENTRY-2324
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java fb826cfa6
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java 1b4fa47dc
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
>
>
> Diff: https://reviews.apache.org/r/68025/diff/2/
>
>
> Testing
> -------
>
> $ mvn -f sentry-service/sentry-service-server/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from 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/68025/
-----------------------------------------------------------
(Updated July 26, 2018, 3:03 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
Changes
-------
Addressing Kalyan's comments
Bugs: SENTRY-2324
https://issues.apache.org/jira/browse/SENTRY-2324
Repository: sentry
Description
-------
Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java fb826cfa6
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java 1b4fa47dc
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
Diff: https://reviews.apache.org/r/68025/diff/2/
Changes: https://reviews.apache.org/r/68025/diff/1-2/
Testing
-------
$ mvn -f sentry-service/sentry-service-server/pom.xml test
Thanks,
Arjun Mishra
Re: Review Request 68025: SENTRY-2324: Allow sentry to fetch
configurable notifications from 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/68025/
-----------------------------------------------------------
(Updated July 23, 2018, 9:50 p.m.)
Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
Bugs: SENTRY-2324
https://issues.apache.org/jira/browse/SENTRY-2324
Repository: sentry
Description
-------
Currently sentry fetches all notifications greater than latest notification Id from HMS. With this sentry will only fetch notifications over a configured depth
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java a9afb151a
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java 7667c47d5
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 8490d7ac2
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java 83a1becd4
Diff: https://reviews.apache.org/r/68025/diff/1/
Testing
-------
$ mvn -f sentry-service/sentry-service-server/pom.xml test
Thanks,
Arjun Mishra