Change Request -- high risk, high reward
Toshio Kuratomi
a.badger at gmail.com
Sun Mar 15 05:40:17 UTC 2009
mmcgrath at redhat.com wrote:
>
> On Mar 14, 2009, at 8:27 PM, Nigel Jones <nigjones at redhat.com> wrote:
>
>>
>> ----- "Toshio Kuratomi" <a.badger at 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_key).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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/fedora-infrastructure-list/attachments/20090314/21a7088f/attachment.sig>
More information about the Fedora-infrastructure-list
mailing list