Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Arjun Mishra via Review Board]

> On Aug. 22, 2018, 5:37 p.m., kalyan kumar kalvagadda wrote:
> > Arjun,
> > 
> > I see you added fetching groups in a lot of API's. Do we know if there wil 
> > be performence benifit in all the cases to to have fetch plan and avoid 
> > loose fetching?
> 
> Na Li wrote:
> I have worked with Arjun yesterday on the behavior of default fetch 
> group. So far, it seems when there are more than 2 fields specifies 
> "default-fetch-group="true"", only the first two fields are bulk-fetched 
> automatically. The other fields are fetched on demand, and cause performance 
> issue. We will work more to confirm this behavior. If it is true, then it is 
> safer to specify fetching group to make sure those fields are fetched in bulk 
> as it does not cause cause harm, but to guarantee good performance.

Hi Kalyan, yes its been tested on operations SHOW ROLES, SHOW TABLES. It 
speeded operation significantly


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/#review207758
---


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> ---
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Na Li via Review Board]

> On Aug. 22, 2018, 5:37 p.m., kalyan kumar kalvagadda wrote:
> > Arjun,
> > 
> > I see you added fetching groups in a lot of API's. Do we know if there wil 
> > be performence benifit in all the cases to to have fetch plan and avoid 
> > loose fetching?

I have worked with Arjun yesterday on the behavior of default fetch group. So 
far, it seems when there are more than 2 fields specifies 
"default-fetch-group="true"", only the first two fields are bulk-fetched 
automatically. The other fields are fetched on demand, and cause performance 
issue. We will work more to confirm this behavior. If it is true, then it is 
safer to specify fetching group to make sure those fields are fetched in bulk 
as it does not cause cause harm, but to guarantee good performance.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/#review207758
---


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> ---
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/#review207698
---


Ship it!




Ship It!

- Na Li


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> ---
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Na Li via Review Board]

> On Aug. 21, 2018, 8:34 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 365 (patched)
> > 
> >
> > privileges is in default fetch group of MSentryUser
> > 
> >  > detachable="true">
> >   
> > 
> >   
> >   
> > 
> > 
> >   
> >   
> > 
> >   
> > 
> >   
> >   > element-type="org.apache.sentry.provider.db.service.model.MSentryRole"/>
> >   
> > 
> >> default-fetch-group="true">
> 
> Arjun Mishra wrote:
> That means the attributes of privileges will be fetched in 
> default-fetch-group, but not the privilege object when fetching sentry_users

agree. Putting "privileges" in default fetch group may only let "privileges" to 
be fetched automatically, but does not allow the content of each privilege to 
be fetched. We are dealing 3 tables here, SENTRY_USER, 
SENTRY_USER_DB_PRIVILEGE_MAP, and SENTRY_DB_PRIVILEGE.


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/#review207690
---


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> ---
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Na Li via Review Board]

> On Aug. 21, 2018, 8:34 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 2239 (original), 2280 (patched)
> > 
> >
> > can you reduce overhead by
> > 
> > result.add(convertToTsentryRole( role, mgroup.getGroupName))? 
> > 
> >   private TSentryRole convertToTSentryRole(MSentryRole mSentryRole, 
> > String groupName) {
> > String roleName = mSentryRole.getRoleName().intern();
> > 
> > Set sentryGroups = new HashSet<>(1);
> > sentryGroups.add(new TSentryGroup(groupName.intern());
> >  
> > 
> > 
> > return new TSentryRole(roleName, sentryGroups, 
> > EMPTY_GRANTOR_PRINCIPAL);
> >   }
> >   
> >   In this way, you don't need to need to mSentryRole.getGroups(), which 
> > should cause another call to DB to get the groups of a role.
> >   
> >   In this way, we only have DB access to get a group and its roles. No 
> > fetching of privileges and other groups of each role that is associated 
> > with the specific group
> 
> Arjun Mishra wrote:
> I wasn't sure if all groups needed to be fetched for that role. I think I 
> could do that in a different ticket. Let me know if you agree

agree. We can investigate this in another jira


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/#review207690
---


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> ---
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Arjun Mishra via Review Board]

> On Aug. 21, 2018, 8:34 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 328 (patched)
> > 
> >
> > what is the purpose of adding this?
> > 
> > IN package.jdo,
> > 
> >  > detachable="true">
> > 
> > You can see the privileges and other fields are all in default fetching 
> > group
> > 
> > " > default-fetch-group="true">"
> > 
> > So I don't think the code change here will make a difference. Have you 
> > tested that this change does improve performance?

default fetch group only fetches primitive types and String objects but not 
anything else. I did test performance improvement with show roles and show 
tables command and they were significantly faster


> On Aug. 21, 2018, 8:34 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 365 (patched)
> > 
> >
> > privileges is in default fetch group of MSentryUser
> > 
> >  > detachable="true">
> >   
> > 
> >   
> >   
> > 
> > 
> >   
> >   
> > 
> >   
> > 
> >   
> >   > element-type="org.apache.sentry.provider.db.service.model.MSentryRole"/>
> >   
> > 
> >> default-fetch-group="true">

That means the attributes of privileges will be fetched in default-fetch-group, 
but not the privilege object when fetching sentry_users


> On Aug. 21, 2018, 8:34 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 1466 (patched)
> > 
> >
> > This will make roles fetched for privileges automatically

It should make privilege fetch for roles and users


> On Aug. 21, 2018, 8:34 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 2239 (original), 2280 (patched)
> > 
> >
> > can you reduce overhead by
> > 
> > result.add(convertToTsentryRole( role, mgroup.getGroupName))? 
> > 
> >   private TSentryRole convertToTSentryRole(MSentryRole mSentryRole, 
> > String groupName) {
> > String roleName = mSentryRole.getRoleName().intern();
> > 
> > Set sentryGroups = new HashSet<>(1);
> > sentryGroups.add(new TSentryGroup(groupName.intern());
> >  
> > 
> > 
> > return new TSentryRole(roleName, sentryGroups, 
> > EMPTY_GRANTOR_PRINCIPAL);
> >   }
> >   
> >   In this way, you don't need to need to mSentryRole.getGroups(), which 
> > should cause another call to DB to get the groups of a role.
> >   
> >   In this way, we only have DB access to get a group and its roles. No 
> > fetching of privileges and other groups of each role that is associated 
> > with the specific group

I wasn't sure if all groups needed to be fetched for that role. I think I could 
do that in a different ticket. Let me know if you agree


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/#review207690
---


On Aug. 21, 2018, 7:19 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61863/
> ---
> 
> (Updated Aug. 21, 2018, 7:19 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Right now when we get privileges from sentry, we pass in a provider like set 
> of groups. Then we create a MSentryGroup object for each group and then get 
> roles using the .getRoles() method. This is bad since we only need the 
> roleNames for the group and not the entire Role object.  
> Instead running a SQL like query and just getting roleNames will drastically 
> improve performance
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  0ef6a208f 
> 
> 
> Diff: https://reviews.apache.org/r/61863/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 61863: SENTRY-1896 - Optimize retrieving entities by other entity types

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61863/
---

(Updated Aug. 21, 2018, 7:19 p.m.)


Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Summary (updated)
-

SENTRY-1896 - Optimize retrieving entities by other entity types


Repository: sentry


Description
---

Right now when we get privileges from sentry, we pass in a provider like set of 
groups. Then we create a MSentryGroup object for each group and then get roles 
using the .getRoles() method. This is bad since we only need the roleNames for 
the group and not the entire Role object.  
Instead running a SQL like query and just getting roleNames will drastically 
improve performance


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 0ef6a208f 


Diff: https://reviews.apache.org/r/61863/diff/4/


Testing
---


Thanks,

Arjun Mishra