Add priority queue implementation #124
Conversation
Instead of using an integer index into the list of queues, use the queue name itself as the id for mapping purposes.
| def job_done(self, coord_message): | ||
| queue_name = self.get_queue_name_for_zoom(coord_message.coord.zoom) | ||
| if queue_name is None: | ||
| # TODO log? should this assert instead? |
There was a problem hiding this comment.
I think we should assert here. Presumably this would mean that tilequeue has rendered a zoom level for which it wasn't configured, which seems to me like an error condition. Should we assume that get_queue_name_for_zoom will return a valid queue name always, and have an assertion in that method (or use [] instead of get())?
There was a problem hiding this comment.
Added assert in 22ea268.
And upon further thought, I think that you're right that we'd want it to fail fast when it didn't find a queue for a particular input. That suggests either the mapping is wrong/incomplete, or the input coordinate is invalid, which I think we'd want to know about right away. I'll go ahead and make this change.
| type: mem | ||
| name: <sqs queue/queue file name/redis key> | ||
| name: <sqs queue/queue file name/redis key/list of queue names> |
There was a problem hiding this comment.
This list and the list in the line above should be in the same order.
There was a problem hiding this comment.
Good catch, updated in b6df7a3.
| zoom-queue-map: | ||
| # keys are <start-zoom>-<end-zoom> (inclusive) ranges | ||
| # values are the queue name to use for zooms in that range | ||
| 0-10: queue-1 |
There was a problem hiding this comment.
Should the queue names be descriptive of the zooms, or generic?
There was a problem hiding this comment.
This is just sample config, but for what we actually do I was planning on using generic names, in descending priority order, ie 1 is higher priority than 2, and so on. That way if we change our mapping policy in the future we won't have to re-create queues to represent the new configuration.
|
|
||
| def __call__(self, zoom): | ||
| assert isinstance(zoom, int) | ||
| assert 0 <= zoom <= 20 |
There was a problem hiding this comment.
We've standardized around 21 in house styles and Tangram.
| except (ValueError, KeyError): | ||
| assert not 'Invalid zoom range: %s' % zoom_range | ||
|
|
||
| assert (0 <= zoom_start <= 20 and |
| @@ -68,9 +69,12 @@ def read(self, max_to_read=1): | |||
| data = message.get_body() | |||
| coord = deserialize_coord(data) | |||
| if coord is None: | |||
| # log? | |||
| # TODO log? | |||
|
|
||
|
|
||
| def coord_is_valid(coord): | ||
| if coord.zoom < 0 or coord.zoom > 20: |
|
(Discussed zoom 21 > 20 in Slack, we can keep as-is now. File new issue to
track that work as it’s more systemic / x-funcional in nature and doesn’t
directly relate to this PR.)
…On Fri, Dec 2, 2016 at 9:08 AM, Robert Marianski ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In config.yaml.sample <#124>:
> type: mem
- name: <sqs queue/queue file name/redis key>
+ name: <sqs queue/queue file name/redis key/list of queue names>
Good catch, updated in b6df7a3
<b6df7a3>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#124>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA0EO5hQJNDsCeJZqtaasObVeAmI87Ccks5rEFB3gaJpZM4LBtvH>
.
|
|
Tracking the zoom 20 -> 21 bump in https://github.com/mapzen/tile-tasks/issues/152 |
Connects to #18
@zerebubuth, @nvkelso, @iandees could you review please?