Change Request -- high risk, high reward

[mmcgrath at redhat.com]

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.

         -Mike

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