Bug 46130 – The commit-cluster bots still race to lock patch_ids

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

The commit-cluster bots still race to lock patch_ids

[Adam Barth]

Created attachment 68146 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 68146 [details]
Patch

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.

[Adam Barth]

Comment on attachment 68146 [details]
Patch

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

>> 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?

Sadly, no.  That forces things to be pretty ugly.

>> 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.

Ok.  I'll move these to the model.

>> 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.

ok

>> 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.

I don't think you can make lists of complicated things, otherwise I'd agree with you.  If you want to make more complicated structures, I think you're supposed to use the notion of a tree of entities, but that seems like too much overhead for what we need.

[Adam Barth]

Created attachment 68151 [details]
Patch

[Eric Seidel (no email)]

(In reply to comment #3)
> > 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.
> 
> I don't think you can make lists of complicated things, otherwise I'd agree with you.  If you want to make more complicated structures, I think you're supposed to use the notion of a tree of entities, but that seems like too much overhead for what we need.

Yup.  I'm all for little-overhead.  Just attempting to vote for model cleanliness.  When someone else writes code to show the list of current locks, they shouldn't have to walk two separate lists then too, for example.

[Eric Seidel (no email)]

Comment on attachment 68151 [details]
Patch

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

> WebKitTools/QueueStatusServer/model/activeworkitems.py:48
> +                nonexpired_item_ids.append(self.item_ids[i])
> +                nonexpired_item_dates.append(self.item_dates[i])

I think we should consider hiding these two lists behind a list of tuples instead.  Then the "API" for dealing with them is to get/set a list of tuples and their storage is a internal detail of this class (even hidden from the classes core logic methods).

[Adam Barth]

Comment on attachment 68151 [details]
Patch

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

>> WebKitTools/QueueStatusServer/model/activeworkitems.py:48
>> +                nonexpired_item_dates.append(self.item_dates[i])
> 
> I think we should consider hiding these two lists behind a list of tuples instead.  Then the "API" for dealing with them is to get/set a list of tuples and their storage is a internal detail of this class (even hidden from the classes core logic methods).

That's good idea, but I'm inclined to do that when we have more than one consumer of that API.

[WebKit Commit Bot]

Comment on attachment 68151 [details]
Patch

Clearing flags on attachment: 68151

Committed r67893: <http://trac.webkit.org/changeset/67893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.