Bug 40389 - make rietveld upload faster and avoid posting to bug on errors
Summary: make rietveld upload faster and avoid posting to bug on errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-09 14:33 PDT by Ojan Vafai
Modified: 2010-06-09 17:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.49 KB, patch)
2010-06-09 14:36 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2010-06-09 15:46 PDT, Ojan Vafai
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-06-09 14:33:14 PDT
make rietveld upload faster and avoid posting to bug on errors
Comment 1 Ojan Vafai 2010-06-09 14:36:08 PDT
Created attachment 58297 [details]
Patch
Comment 2 Adam Barth 2010-06-09 15:26:34 PDT
Comment on attachment 58297 [details]
Patch

Looks good, but R- because of the race condition.

WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:275
 +              return self._fetch_bug(bugs[0]).in_rietveld_queue_patches()[0]
Is there a race condition where the first query sees the bug as having a rietveld? patch but this request doesn't?  Wouldn't that cause use to throw an exception here?

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:338
 +          self._reject_patch(patch.id(), message)
I'd pass the patch here for consistency with other queues.
Comment 3 Ojan Vafai 2010-06-09 15:46:09 PDT
Created attachment 58301 [details]
Patch
Comment 4 Ojan Vafai 2010-06-09 15:47:33 PDT
(In reply to comment #2)
> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:275
>  +              return self._fetch_bug(bugs[0]).in_rietveld_queue_patches()[0]
> Is there a race condition where the first query sees the bug as having a rietveld? patch but this request doesn't?  Wouldn't that cause use to throw an exception here?

Whoops. Fixed.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:338
>  +          self._reject_patch(patch.id(), message)
> I'd pass the patch here for consistency with other queues.

Actually, it seems like all the other queues pass the patch id around in the functions called from handle_script_error and handle_unexpected_error.
Comment 5 Adam Barth 2010-06-09 16:37:08 PDT
Comment on attachment 58301 [details]
Patch

I think you have a typo, but looks good otherwise.

WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:274
 +          def fetch_patch(url):
Normally we'd make this a private method.

WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:275
 +              bugs = self._fetch_bug_ids_advanced_query(rietveld_queue_url)
rietveld_queue_url -> url?

WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:276
 +              patches = self._fetch_bug(bugs[0]).in_rietveld_queue_patches()
Does in_rietveld_queue_patches cover r? too?
Comment 6 Ojan Vafai 2010-06-09 17:10:37 PDT
(In reply to comment #5)
> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:274
>  +          def fetch_patch(url):
> Normally we'd make this a private method.

Done.

> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:275
>  +              bugs = self._fetch_bug_ids_advanced_query(rietveld_queue_url)
> rietveld_queue_url -> url?
> 
> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:276
>  +              patches = self._fetch_bug(bugs[0]).in_rietveld_queue_patches()
> Does in_rietveld_queue_patches cover r? too?

Doh. Sorry, I didn't get around to running the queue through all the in-rietveld? patches and didn't hit this. Anyways, fixed both issues and confirmed that r? patches get uploaded correctly.
Comment 7 Ojan Vafai 2010-06-09 17:28:14 PDT
Committed r60924: <http://trac.webkit.org/changeset/60924>