FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Fedora Infrastructure

 
 
LinkBack Thread Tools
 
Old 03-14-2009, 11:36 PM
Toshio Kuratomi
 
Default Change Request -- high risk, high reward

ricky and I have identified a piece of code in fas2's new safasprovider
that's querying the database a lot. It's causing anything that checks
identity to issue a query to the database to lookup the visit cookie.
This is causing things that lookup information on the identity multiple
times to take a lot of time. One of the functions that does this is
filter_private(), the function that removes excess information according
to privacy settings and who is looking for the data. Our test was the
/user/list method which is currently running over 5 minutes for an
otherwise non-database loop. Changing this caused the loop to run for 8
seconds.

That's the benefit. The risk is that this is a change to the
safasprovider, ie the portion of fas2 that authenticates the user. So
if there's a reason this shouldn't be cached, we could potentially be
breaking a lot of things.

Ricky and I have both looked at the code in
fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
this. The TG-1.0.8 saprovider on which safasprovider is based does not
cache this but I've looked at the code and it seems like their provider
only uses the variable in question a maximum of two times during a
request. The CSRF protection that we've enabled needs to use this
variable more often.

Here's the code:

--- a/fas/safasprovider.py
+++ b/fas/safasprovider.py
@@ -65,6 +65,7 @@ class SaFasIdentity(object):

def __init__(self, visit_key=None, user=None, using_ssl=False):
self.visit_key = visit_key
+ self._visit_link = None
if user:
self._user = user
if visit_key is not None:
@@ -201,9 +202,13 @@ class SaFasIdentity(object):
### TG: Same as TG-1.0.8
def _get_visit_link(self):
'Get the visit link to this identity.'
+ if self._visit_link:
+ return self.visit_link
if self.visit_key is None:
- return None
- return
visit_class.query.filter_by(visit_key=self.visit_k ey).first()
+ self._visit_link = None
+ else:
+ self._visit_link =
visit_class.query.filter_by(visit_key=self.visi
t_key).first()
+ return self._visit_link
visit_link = property(_get_visit_link)

If we were outside of freeze, I would apply this as it is causing issues
for some of the things that talk to fas (like zodbot and developer
instances of pkgdb).

-Toshio

_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
 
Old 03-14-2009, 11:50 PM
Ricky Zhou
 
Default Change Request -- high risk, high reward

On 2009-03-14 05:36:27 PM, Toshio Kuratomi wrote:
> Ricky and I have both looked at the code in
> fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
> this. The TG-1.0.8 saprovider on which safasprovider is based does not
> cache this but I've looked at the code and it seems like their provider
> only uses the variable in question a maximum of two times during a
> request. The CSRF protection that we've enabled needs to use this
> variable more often.
>
> Here's the code:
>
> --- a/fas/safasprovider.py
> +++ b/fas/safasprovider.py
> @@ -65,6 +65,7 @@ class SaFasIdentity(object):
>
> def __init__(self, visit_key=None, user=None, using_ssl=False):
> self.visit_key = visit_key
> + self._visit_link = None
> if user:
> self._user = user
> if visit_key is not None:
> @@ -201,9 +202,13 @@ class SaFasIdentity(object):
> ### TG: Same as TG-1.0.8
> def _get_visit_link(self):
> 'Get the visit link to this identity.'
> + if self._visit_link:
> + return self.visit_link
I already mentioned this to Toshio, but this line should be changed
to return self._visit_link

> if self.visit_key is None:
> - return None
> - return
> visit_class.query.filter_by(visit_key=self.visit_k ey).first()
> + self._visit_link = None
> + else:
> + self._visit_link =
> visit_class.query.filter_by(visit_key=self.visi
> t_key).first()
> + return self._visit_link
> visit_link = property(_get_visit_link)
>
> If we were outside of freeze, I would apply this as it is causing issues
> for some of the things that talk to fas (like zodbot and developer
> instances of pkgdb).
+1

As Toshio mentioned, we've looked at the places where this variable is
used, and it should be safe (and easy to revert otherwise). This will
be a giant performance improvement for code where we call filter_private
on a lot of users (which is a lot of places).

Thanks,
Ricky
_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
 
Old 03-15-2009, 12:15 AM
Jon Stanley
 
Default Change Request -- high risk, high reward

I'm out, sorry for the top post. But zodbot FAS routines are not
functional due to this, so I'd be +1 here if I had a vote

On 3/14/09, Ricky Zhou <ricky@fedoraproject.org> wrote:
> On 2009-03-14 05:36:27 PM, Toshio Kuratomi wrote:
>> Ricky and I have both looked at the code in
>> fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
>> this. The TG-1.0.8 saprovider on which safasprovider is based does not
>> cache this but I've looked at the code and it seems like their provider
>> only uses the variable in question a maximum of two times during a
>> request. The CSRF protection that we've enabled needs to use this
>> variable more often.
>>
>> Here's the code:
>>
>> --- a/fas/safasprovider.py
>> +++ b/fas/safasprovider.py
>> @@ -65,6 +65,7 @@ class SaFasIdentity(object):
>>
>> def __init__(self, visit_key=None, user=None, using_ssl=False):
>> self.visit_key = visit_key
>> + self._visit_link = None
>> if user:
>> self._user = user
>> if visit_key is not None:
>> @@ -201,9 +202,13 @@ class SaFasIdentity(object):
>> ### TG: Same as TG-1.0.8
>> def _get_visit_link(self):
>> 'Get the visit link to this identity.'
>> + if self._visit_link:
>> + return self.visit_link
> I already mentioned this to Toshio, but this line should be changed
> to return self._visit_link
>
>> if self.visit_key is None:
>> - return None
>> - return
>> visit_class.query.filter_by(visit_key=self.visit_k ey).first()
>> + self._visit_link = None
>> + else:
>> + self._visit_link =
>> visit_class.query.filter_by(visit_key=self.visi
>> t_key).first()
>> + return self._visit_link
>> visit_link = property(_get_visit_link)
>>
>> If we were outside of freeze, I would apply this as it is causing issues
>> for some of the things that talk to fas (like zodbot and developer
>> instances of pkgdb).
> +1
>
> As Toshio mentioned, we've looked at the places where this variable is
> used, and it should be safe (and easy to revert otherwise). This will
> be a giant performance improvement for code where we call filter_private
> on a lot of users (which is a lot of places).
>
> Thanks,
> Ricky
>

--
Sent from my mobile device

_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
 
Old 03-15-2009, 12:27 AM
Nigel Jones
 
Default Change Request -- high risk, high reward

----- "Toshio Kuratomi" <a.badger@gmail.com> wrote:

> ricky and I have identified a piece of code in fas2's new
> safasprovider
> that's querying the database a lot. It's causing anything that
> checks
> identity to issue a query to the database to lookup the visit cookie.
> This is causing things that lookup information on the identity
> multiple
> times to take a lot of time. One of the functions that does this is
> filter_private(), the function that removes excess information
> according
> to privacy settings and who is looking for the data. Our test was
> the
> /user/list method which is currently running over 5 minutes for an
> otherwise non-database loop. Changing this caused the loop to run for
> 8
> seconds.
>
> That's the benefit. The risk is that this is a change to the
> safasprovider, ie the portion of fas2 that authenticates the user.
> So
> if there's a reason this shouldn't be cached, we could potentially be
> breaking a lot of things.
>
> Ricky and I have both looked at the code in
> fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
> this. The TG-1.0.8 saprovider on which safasprovider is based does
> not
> cache this but I've looked at the code and it seems like their
> provider
> only uses the variable in question a maximum of two times during a
> request. The CSRF protection that we've enabled needs to use this
> variable more often.
>
> Here's the code:
>
> --- a/fas/safasprovider.py
> +++ b/fas/safasprovider.py
> @@ -65,6 +65,7 @@ class SaFasIdentity(object):
>
> def __init__(self, visit_key=None, user=None, using_ssl=False):
> self.visit_key = visit_key
> + self._visit_link = None
> if user:
> self._user = user
> if visit_key is not None:
> @@ -201,9 +202,13 @@ class SaFasIdentity(object):
> ### TG: Same as TG-1.0.8
> def _get_visit_link(self):
> 'Get the visit link to this identity.'
> + if self._visit_link:
> + return self.visit_link
> if self.visit_key is None:
> - return None
> - return
> visit_class.query.filter_by(visit_key=self.visit_k ey).first()
> + self._visit_link = None
> + else:
> + self._visit_link =
> visit_class.query.filter_by(visit_key=self.visi
> t_key).first()
> + return self._visit_link
> visit_link = property(_get_visit_link)
>
> If we were outside of freeze, I would apply this as it is causing
> issues
> for some of the things that talk to fas (like zodbot and developer
> instances of pkgdb).
>
> -Toshio
+1 - I live for danger!

That said, anything that speeds it up is a good thing!
>
>
> _______________________________________________
> Fedora-infrastructure-list mailing list
> Fedora-infrastructure-list@redhat.com
> https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list

_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
 
Old 03-15-2009, 02:57 AM
 
Default Change Request -- high risk, high reward

On Mar 14, 2009, at 8:27 PM, Nigel Jones <nigjones@redhat.com> wrote:



----- "Toshio Kuratomi" <a.badger@gmail.com> wrote:


ricky and I have identified a piece of code in fas2's new
safasprovider
that's querying the database a lot. It's causing anything that
checks
identity to issue a query to the database to lookup the visit cookie.
This is causing things that lookup information on the identity
multiple
times to take a lot of time. One of the functions that does this is
filter_private(), the function that removes excess information
according
to privacy settings and who is looking for the data. Our test was
the
/user/list method which is currently running over 5 minutes for an
otherwise non-database loop. Changing this caused the loop to run
for

8
seconds.

That's the benefit. The risk is that this is a change to the
safasprovider, ie the portion of fas2 that authenticates the user.
So
if there's a reason this shouldn't be cached, we could potentially be
breaking a lot of things.

Ricky and I have both looked at the code in
fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
this. The TG-1.0.8 saprovider on which safasprovider is based does
not
cache this but I've looked at the code and it seems like their
provider
only uses the variable in question a maximum of two times during a
request. The CSRF protection that we've enabled needs to use this
variable more often.

Here's the code:

--- a/fas/safasprovider.py
+++ b/fas/safasprovider.py
@@ -65,6 +65,7 @@ class SaFasIdentity(object):

def __init__(self, visit_key=None, user=None, using_ssl=False):
self.visit_key = visit_key
+ self._visit_link = None
if user:
self._user = user
if visit_key is not None:
@@ -201,9 +202,13 @@ class SaFasIdentity(object):
### TG: Same as TG-1.0.8
def _get_visit_link(self):
'Get the visit link to this identity.'
+ if self._visit_link:
+ return self.visit_link
if self.visit_key is None:
- return None
- return
visit_class.query.filter_by(visit_key=self.visit_k ey).first()
+ self._visit_link = None
+ else:
+ self._visit_link =
visit_class.query.filter_by(visit_key=self.visi
t_key).first()
+ return self._visit_link
visit_link = property(_get_visit_link)

If we were outside of freeze, I would apply this as it is causing
issues
for some of the things that talk to fas (like zodbot and developer
instances of pkgdb).

-Toshio

+1 - I live for danger!

That said, anything that speeds it up is a good thing!


+1. Just the beta freezeand the revert is easy.

-Mike






_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list


_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list


_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
 
Old 03-15-2009, 04:40 AM
Toshio Kuratomi
 
Default Change Request -- high risk, high reward

mmcgrath@redhat.com wrote:
>
> On Mar 14, 2009, at 8:27 PM, Nigel Jones <nigjones@redhat.com> wrote:
>
>>
>> ----- "Toshio Kuratomi" <a.badger@gmail.com> wrote:
>>
>>> ricky and I have identified a piece of code in fas2's new
>>> safasprovider
>>> that's querying the database a lot. It's causing anything that
>>> checks
>>> identity to issue a query to the database to lookup the visit cookie.
>>> This is causing things that lookup information on the identity
>>> multiple
>>> times to take a lot of time. One of the functions that does this is
>>> filter_private(), the function that removes excess information
>>> according
>>> to privacy settings and who is looking for the data. Our test was
>>> the
>>> /user/list method which is currently running over 5 minutes for an
>>> otherwise non-database loop. Changing this caused the loop to run for
>>> 8
>>> seconds.
>>>
>>> That's the benefit. The risk is that this is a change to the
>>> safasprovider, ie the portion of fas2 that authenticates the user.
>>> So
>>> if there's a reason this shouldn't be cached, we could potentially be
>>> breaking a lot of things.
>>>
>>> Ricky and I have both looked at the code in
>>> fas/safasprovider.py::SaFasIdentity and think that it's safe to cache
>>> this. The TG-1.0.8 saprovider on which safasprovider is based does
>>> not
>>> cache this but I've looked at the code and it seems like their
>>> provider
>>> only uses the variable in question a maximum of two times during a
>>> request. The CSRF protection that we've enabled needs to use this
>>> variable more often.
>>>
>>> Here's the code:
>>>
>>> --- a/fas/safasprovider.py
>>> +++ b/fas/safasprovider.py
>>> @@ -65,6 +65,7 @@ class SaFasIdentity(object):
>>>
>>> def __init__(self, visit_key=None, user=None, using_ssl=False):
>>> self.visit_key = visit_key
>>> + self._visit_link = None
>>> if user:
>>> self._user = user
>>> if visit_key is not None:
>>> @@ -201,9 +202,13 @@ class SaFasIdentity(object):
>>> ### TG: Same as TG-1.0.8
>>> def _get_visit_link(self):
>>> 'Get the visit link to this identity.'
>>> + if self._visit_link:
>>> + return self.visit_link
>>> if self.visit_key is None:
>>> - return None
>>> - return
>>> visit_class.query.filter_by(visit_key=self.visit_k ey).first()
>>> + self._visit_link = None
>>> + else:
>>> + self._visit_link =
>>> visit_class.query.filter_by(visit_key=self.visi
>>> t_key).first()
>>> + return self._visit_link
>>> visit_link = property(_get_visit_link)
>>>
>>> If we were outside of freeze, I would apply this as it is causing
>>> issues
>>> for some of the things that talk to fas (like zodbot and developer
>>> instances of pkgdb).
>>>
>>> -Toshio
>> +1 - I live for danger!
>>
>> That said, anything that speeds it up is a good thing!
>
> +1. Just the beta freezeand the revert is easy.
>
Thanks guys. Hotfixed on the server. If anyone notices wierdness with
authentication to fas, let me know.

-Toshio

_______________________________________________
Fedora-infrastructure-list mailing list
Fedora-infrastructure-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
 

Thread Tools




All times are GMT. The time now is 12:04 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org