Summary

Speed up rbt post -u.

Review Request #5099 — Created Dec. 11, 2013 and submitted Dec. 13, 2013, 5:24 p.m.

Information

Owner

chipx86

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Speed up rbt post -u.

rbt post -u, in an effort to match draft summaries, was fetching every
opened draft for a user individually and pulling info from them. This
was slow, particularly with lots of pending review requests.

Now it uses the field expansion support in the API to fetch drafts in
one go, reducing the number of calls significantly.

Testing Done

Ran with --debug and saw that it wasn't fetching drafts anymore.

Tested with and without pending drafts. It matched correctly.

Issues

Description	From	Last Updated
access to a Resource's 'fields' attribute is something we shouldn't really be doing. The fact that it's not "private" is …	SM smacleod	Dec. 12, 2013, 12:55 p.m.
Will this work properly? Remember, as of now ".draft" isn't a real dictionary. I don't believe I've defined a __nonzero__ …	SM smacleod	Dec. 12, 2013, 6:14 p.m.

rbtools/commands/post.py (Diff revision 1)

You can access the data from a resource using both attributes,
or key lookup:

review_request.draft

vs:

review_request['draft']

So, if you prefer this conditional could be written like so:

if 'draft' in review_request:

Just a heads up. Go with whatever you prefer.

david Dec. 11, 2013, 3:19 p.m.
```
Using 'in' seems way more pythonic.
```

chipx86 Dec. 12, 2013, 12:55 p.m.

So it turns out I had a bug here anyway. Because we expand the draft, there's always going to be a draft parameter. Instead, we want to check if it's empty or not.

rbtools/commands/post.py (Diff revision 1)

The issue has been resolved. Show all issues

access to a Resource's 'fields' attribute is something we shouldn't
really be doing. The fact that it's not "private" is just a mistake
I made when restructuring the whole transport stuff. We should really
rename it "_fields" and not touch it.

You will have the same effect if you change this to:

fields = review_request

Change Summary:


Fixed the lookup for drafts.
Stopped using fields

Diff:

Revision 2 (+7 -8)

Show changes

rbtools/commands/post.py

rbtools/commands/post.py (Diff revision 2)

The issue has been dropped. Show all issues

Will this work properly? Remember, as of now ".draft" isn't a real dictionary.
I don't believe I've defined a __nonzero__ method on it so you might want to
verify this.

chipx86 Dec. 12, 2013, 2:42 p.m.

The way it's returned in the API, it's actually a list of dictionaries (of which there will be at most 1). So I'm testing if it's empty, basically.

It seems to work (I posted a change to one with a draft, and posted an update to this review request without a draft), but I can spend more time playing with it.

smacleod Dec. 12, 2013, 6:14 p.m.

Bah, yeah, that's me brain farting. Since it's a list (and the ResourceListField's act correctly) it should work as expected.

Ship it!

```
Ship It!
```

Status:: Completed