[webkit-reviews] review denied: [Bug 46130] The commit-cluster bots still race to lock patch_ids : [Attachment 68146] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 46130: The commit-cluster bots still race to lock patch_ids
https://bugs.webkit.org/show_bug.cgi?id=46130

Attachment 68146: Patch
https://bugs.webkit.org/attachment.cgi?id=68146&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68146&action=review

In general I think this is a great change.  But I think it could be factored
cleaner.

> WebKitTools/QueueStatusServer/handlers/nextpatch.py:41
> +    one_hour_ago = time.mktime((now - timedelta(minutes=60)).timetuple())

Wow.  What a confusing way to make this time object.  Amazing there isn't a
cleaner way.

Doesn't the model list type understand dates?

> WebKitTools/QueueStatusServer/handlers/nextpatch.py:49
> +    nonexpired_item_ids = []
> +    nonexpired_item_dates = []
> +    for i in range(len(active_work_items.item_ids)):
> +	   if active_work_items.item_dates[i] > one_hour_ago:
> +	       nonexpired_item_ids.append(active_work_items.item_ids[i])
> +	       nonexpired_item_dates.append(active_work_items.item_dates[i])
> +    active_work_items.item_ids = nonexpired_item_ids
> +    active_work_items.item_dates = nonexpired_item_dates

I would think this should just walk the list and remove items.	Or use an
accessor on the model which returns tuples.  I don't think all the code which
deals with the model needs to be exposed to the fact that these are stored as
separate lists.

Maybe this whole function should be made a static method on the model?

> WebKitTools/QueueStatusServer/handlers/nextpatch.py:52
> +def _find_ready_item(active_work_items, work_item_ids, now):

Since there is no difference between a free function and a static method, seems
a static method would be a better choice here.

I think you can even use a non-static method and python may automatically bind
self for you when you do self.my_method_name to pass to the transaction.

> WebKitTools/QueueStatusServer/model/activeworkitems.py:34
> +class ActiveWorkItems(WorkItems):

I dont' think this makes sense as a WorkItems class.  It wants to have
different storage anyway.

> WebKitTools/QueueStatusServer/model/activeworkitems.py:35
> +    item_dates = db.ListProperty(float)

Seems this should end up being a list of tuples, no?  number, date pairs? 
Maybe such a thing doesn't exist for the model.  In which case, shouldn't we
use accessor methods to access these in pairs. Then we don't need to have all
the individual code paths be careful to access both.