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/11/16 19:29:49 UTC
Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore
has references to MAuthzPathsMapping when it should be generic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63886/
-----------------------------------------------------------
Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
Repository: sentry
Description
-------
Current isTableEmpty implementation is below
private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
Query query = pm.newQuery(clazz);
query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
// setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
// and Oracle (using the ROWNUM keyword), with the query only finding the objects required
// by the user directly in the datastore. For other RDBMS the query will retrieve all
// objects up to the "to" record, and will not pass any unnecessary objects that are before
// the "from" record.
query.setRange(0, 1);
return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
}
We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
Diff: https://reviews.apache.org/r/63886/diff/1/
Testing
-------
mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
Thanks,
Arjun Mishra
Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore has references to MAuthzPathsMapping when it should be generic
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/63886/#review191251
-----------------------------------------------------------
Ship it!
Ship It!
- Vadim Spector
On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63886/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:29 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Current isTableEmpty implementation is below
> private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
> Query query = pm.newQuery(clazz);
> query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> // setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
> // and Oracle (using the ROWNUM keyword), with the query only finding the objects required
> // by the user directly in the datastore. For other RDBMS the query will retrieve all
> // objects up to the "to" record, and will not pass any unnecessary objects that are before
> // the "from" record.
> query.setRange(0, 1);
> return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
> }
> We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
>
>
> Diff: https://reviews.apache.org/r/63886/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore has references to MAuthzPathsMapping when it should be generic
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/63886/#review191352
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63886/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:29 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Current isTableEmpty implementation is below
> private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
> Query query = pm.newQuery(clazz);
> query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> // setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
> // and Oracle (using the ROWNUM keyword), with the query only finding the objects required
> // by the user directly in the datastore. For other RDBMS the query will retrieve all
> // objects up to the "to" record, and will not pass any unnecessary objects that are before
> // the "from" record.
> query.setRange(0, 1);
> return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
> }
> We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
>
>
> Diff: https://reviews.apache.org/r/63886/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore has references to MAuthzPathsMapping when it should be generic
Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
> On Nov. 16, 2017, 8:14 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3251 (original), 3251 (patched)
> > <https://reviews.apache.org/r/63886/diff/1/?file=1894544#file1894544line3251>
> >
> > So, it means this code only worked for MAuthzPathsMapping type.
> >
> > Which implies we've never tested it for any other type - shouldn't we add such a test now?
>
> Arjun Mishra wrote:
> It is acutally usd by isHmsNotificationEmpt() method, which passes in MSentryHmsNotification.class. I am not sure why an exception was never thrown because this is the first method we execute before taking a full snapshot for the first time.
>
> Test cases were included in TestHMSFollower and they seem to pass. Did you still want me to add tests?
No, actually, nevermind, the way Java generics work, the old casting works even if the type is wrong, because it is a collection. The fix is ok.
- Vadim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63886/#review191238
-----------------------------------------------------------
On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63886/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:29 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Current isTableEmpty implementation is below
> private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
> Query query = pm.newQuery(clazz);
> query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> // setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
> // and Oracle (using the ROWNUM keyword), with the query only finding the objects required
> // by the user directly in the datastore. For other RDBMS the query will retrieve all
> // objects up to the "to" record, and will not pass any unnecessary objects that are before
> // the "from" record.
> query.setRange(0, 1);
> return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
> }
> We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
>
>
> Diff: https://reviews.apache.org/r/63886/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore has references to MAuthzPathsMapping when it should be generic
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
> On Nov. 16, 2017, 8:14 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3251 (original), 3251 (patched)
> > <https://reviews.apache.org/r/63886/diff/1/?file=1894544#file1894544line3251>
> >
> > So, it means this code only worked for MAuthzPathsMapping type.
> >
> > Which implies we've never tested it for any other type - shouldn't we add such a test now?
It is acutally usd by isHmsNotificationEmpt() method, which passes in MSentryHmsNotification.class. I am not sure why an exception was never thrown because this is the first method we execute before taking a full snapshot for the first time.
Test cases were included in TestHMSFollower and they seem to pass. Did you still want me to add tests?
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63886/#review191238
-----------------------------------------------------------
On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63886/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:29 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Current isTableEmpty implementation is below
> private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
> Query query = pm.newQuery(clazz);
> query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> // setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
> // and Oracle (using the ROWNUM keyword), with the query only finding the objects required
> // by the user directly in the datastore. For other RDBMS the query will retrieve all
> // objects up to the "to" record, and will not pass any unnecessary objects that are before
> // the "from" record.
> query.setRange(0, 1);
> return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
> }
> We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
>
>
> Diff: https://reviews.apache.org/r/63886/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore has references to MAuthzPathsMapping when it should be generic
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/63886/#review191238
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3251 (original), 3251 (patched)
<https://reviews.apache.org/r/63886/#comment268934>
So, it means this code only worked for MAuthzPathsMapping type.
Which implies we've never tested it for any other type - shouldn't we add such a test now?
- Vadim Spector
On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63886/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:29 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Current isTableEmpty implementation is below
> private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
> Query query = pm.newQuery(clazz);
> query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> // setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
> // and Oracle (using the ROWNUM keyword), with the query only finding the objects required
> // by the user directly in the datastore. For other RDBMS the query will retrieve all
> // objects up to the "to" record, and will not pass any unnecessary objects that are before
> // the "from" record.
> query.setRange(0, 1);
> return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
> }
> We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
>
>
> Diff: https://reviews.apache.org/r/63886/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
>
>
> Thanks,
>
> Arjun Mishra
>
>
Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in
SentryStore has references to MAuthzPathsMapping when it should be generic
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/63886/#review191272
-----------------------------------------------------------
Ship it!
make sure that tests pass.
- kalyan kumar kalvagadda
On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63886/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:29 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Current isTableEmpty implementation is below
> private boolean isTableEmptyCore(PersistenceManager pm, Class clazz) {
> Query query = pm.newQuery(clazz);
> query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> // setRange is implemented efficiently for MySQL, Postgresql (using the LIMIT SQL keyword)
> // and Oracle (using the ROWNUM keyword), with the query only finding the objects required
> // by the user directly in the datastore. For other RDBMS the query will retrieve all
> // objects up to the "to" record, and will not pass any unnecessary objects that are before
> // the "from" record.
> query.setRange(0, 1);
> return ((List<MAuthzPathsMapping>) query.execute()).isEmpty();
> }
> We seem to be casting query.execute to a List<MAuthzPathsMapping> when there is no need for it
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7217dea7a
>
>
> Diff: https://reviews.apache.org/r/63886/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-tests/sentry-tests-hive/pom.xml test -Dtest=TestPrivilegesAtTableScopePart1
>
>
> Thanks,
>
> Arjun Mishra
>
>