Change Request -- high risk, high reward

[Nigel Jones]

----- "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!
> 
> 
> _______________________________________________
> Fedora-infrastructure-list mailing list
> Fedora-infrastructure-list at redhat.com
> https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list