Bug 47909 - Make patch release explicit and not a magic part of "retry" status
Summary: Make patch release explicit and not a magic part of "retry" status
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: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-19 09:24 PDT by Eric Seidel (no email)
Modified: 2010-10-19 14:20 PDT (History)
1 user (show)

See Also:


Attachments
Patch (13.48 KB, patch)
2010-10-19 09:28 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated per Adam's suggestions (15.06 KB, patch)
2010-10-19 13:21 PDT, Eric Seidel (no email)
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 Eric Seidel (no email) 2010-10-19 09:24:25 PDT
Make patch release explicit and not a magic part of "retry" status
Comment 1 Eric Seidel (no email) 2010-10-19 09:28:06 PDT
Created attachment 71177 [details]
Patch
Comment 2 Adam Barth 2010-10-19 11:25:08 PDT
Comment on attachment 71177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71177&action=review

> WebKitTools/Scripts/webkitpy/common/net/statusserver.py:116
> +    def release_work_item(self, queue_name, patch):
> +        _log.debug("Releasing work item %s from %s" % (patch.id(), queue_name))
> +        release_patch_url = "%s/release-patch/%s" % (self.url, queue_name)
> +        self._browser.open(release_patch_url)
> +        self._browser.select_form(name="release_patch")
> +        self._browser["attachment_id"] = patch.id()
> +        self._browser.submit()

We need a network transaction for writing to appengine

> WebKitTools/Scripts/webkitpy/tool/mocktool.py:551
> +    def release_work_item(self, queue_name, patch):
> +        pass

Should we log so we can test that this happens?
Comment 3 Eric Seidel (no email) 2010-10-19 13:21:39 PDT
Created attachment 71197 [details]
Updated per Adam's suggestions
Comment 4 Adam Barth 2010-10-19 13:39:15 PDT
Comment on attachment 71197 [details]
Updated per Adam's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=71197&action=review

> WebKitTools/QueueStatusServer/handlers/nextpatch.py:39
> +    # FIXME: This should probably be a post, or an explict lock_patch
> +    # since GET requests shouldn't really modify the datastore.

Good point.

> WebKitTools/QueueStatusServer/handlers/releasepatch.py:46
> +        # FIXME: This queue lookup should be shared between handlers.
> +        queue = Queue.queue_with_name(queue_name)
> +        if not queue:
> +            self.error(404)
> +            return

Yeah, feels like a base class to me.

> WebKitTools/QueueStatusServer/handlers/releasepatch.py:53
> +        if not latest_status:
> +            self.error(404)
> +            return

When would this ever happen?

> WebKitTools/QueueStatusServer/handlers/releasepatch.py:56
> +        # Ideally we should use a transaction for the calls to
> +        # WorkItems and ActiveWorkItems.

Yeah, I don't know if this will be a problem.  I guess we'll find out.  We could ask some AppEngine experts.  I suspect the concurrency model doesn't give you a total ordering between transactions.  I asked Mihai, but he didn't know.
Comment 5 Adam Barth 2010-10-19 13:39:43 PDT
Comment on attachment 71197 [details]
Updated per Adam's suggestions

Actually, this requires a queue reboot, right?
Comment 6 Eric Seidel (no email) 2010-10-19 13:43:47 PDT
I mean, any queue change does.  If the queues don't reboot, nothing will really change except that retries will be delayed until the lock expires.  I'll land it manually and reboot my local queue.
Comment 7 Adam Barth 2010-10-19 14:10:48 PDT
You should be able to reboot mine.  Look for a command window that's running the commit-queue.  The command I use is start-commit-queue.sh.  It should be in the bash history.
Comment 8 Eric Seidel (no email) 2010-10-19 14:20:20 PDT
Committed r70088: <http://trac.webkit.org/changeset/70088>