[webkit-reviews] review granted: [Bug 47909] Make patch release explicit and not a magic part of "retry" status : [Attachment 71197] Updated per Adam's suggestions

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 47909: Make patch release explicit and not a magic part of "retry" status
https://bugs.webkit.org/show_bug.cgi?id=47909

Attachment 71197: Updated per Adam's suggestions
https://bugs.webkit.org/attachment.cgi?id=71197&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.