Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in SentryStore has references to MAuthzPathsMapping when it should be generic

[Na Li via Review Board]

---
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) query.execute()).isEmpty();
>   }
> We seem to be casting query.execute to a List 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

[kalyan kumar kalvagadda via Review Board]

---
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) query.execute()).isEmpty();
>   }
> We seem to be casting query.execute to a List 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

[Vadim Spector via Review Board]

> 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)
> > 
> >
> > 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) query.execute()).isEmpty();
>   }
> We seem to be casting query.execute to a List 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

[Vadim Spector via Review Board]

---
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) query.execute()).isEmpty();
>   }
> We seem to be casting query.execute to a List 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

[Arjun Mishra via Review Board]

> 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)
> > 
> >
> > 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) query.execute()).isEmpty();
>   }
> We seem to be casting query.execute to a List 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

[Vadim Spector via Review Board]

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


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) query.execute()).isEmpty();
>   }
> We seem to be casting query.execute to a List 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
> 
>