Bug 40389

Summary:

make rietveld upload faster and avoid posting to bug on errors

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

New Bugs

Assignee:

Ojan Vafai <ojan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cjerdonek, eric, jparent

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	abarth: review+, abarth: commit-queue-

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>