Configurable up-zooming of tiles from Redshift #311
Conversation
This changes the TOI gardener/pruner to parse the tile size and use the configured metatile size to up-zoom access logs to the metatile they're part of.
| assert tile_size_as_zoom <= cfg.metatile_zoom | ||
| tile_zoom_offset = tile_size_as_zoom - cfg.metatile_zoom | ||
|
|
||
| except AssertionError: |
There was a problem hiding this comment.
Would this be better as an if block rather than a try/assert/except?
There was a problem hiding this comment.
Yes, if that were the only assertion. Unfortunately, there's also one in metatile_zoom_from_size, called by metatile_zoom_from_str, which asserts that the metatile size is a power of two. I don't think it's possible to get a (200 OK) entry in the database like that, but I think we wouldn't want this process to stop in that case.
I could rewrite both functions to return None on error, and check for that. Do you think that would be better than the try/except?
There was a problem hiding this comment.
Ah, didn't see the other assertion. 👍
There was a problem hiding this comment.
Catching an assertion error seems odd to me as well, and perhaps it would be better to return None instead and check for that at call sites.
By the way, it doesn't look like we're filtering on successful responses here, or I just missed it. Should we be?
There was a problem hiding this comment.
By successful responses, I just meant that we'd only consider 200 and 304 responses, because we could get garbage with 404s. But, the query looks like it'll guarantee that we always get valid coordinates, so on second thought maybe it's better to just leave the way it is. After all, we are interested in the requested coordinates here, regardless of what the response looked like, eg 5xx responses should be considered.
Never mind! :)
| assert tile_size_as_zoom <= cfg.metatile_zoom | ||
| tile_zoom_offset = tile_size_as_zoom - cfg.metatile_zoom | ||
|
|
||
| except AssertionError: |
There was a problem hiding this comment.
Catching an assertion error seems odd to me as well, and perhaps it would be better to return None instead and check for that at call sites.
By the way, it doesn't look like we're filtering on successful responses here, or I just missed it. Should we be?
This changes the TOI gardener/pruner to parse the tile size and use the configured metatile size to up-zoom access logs to the metatile they're part of.