Move tiles of interest set out of Redis and into S3 #193

iandees · 2017-04-25T23:29:14Z

Moves the tiles of interest set out of Redis and into S3.

nvkelso · 2017-04-26T15:08:31Z

tilequeue/command.py

+    logger.info('Enqueueing to SQS ... done')
+
+    if cfg.seed_should_add_to_tiles_of_interest:
+        logger.info('Adding to Redis ... ')


Per the PR title, is Redis still needed here?

This was a terminology problem (should have said "Tiles of Interest" not "Redis").

Fixed in 7a52e76

nvkelso · 2017-04-26T15:10:20Z

Do we want to allow read/write to local disk for the TOI (not just S3)?

iandees · 2017-04-26T15:13:13Z

This isn't ready for review yet. I still have lots to go :) These (and other things that Rob brought up with me) are all good points.

nvkelso · 2017-04-26T15:19:57Z

Woot! I've added WIP: to the PR title to make that clear.

…' helper Add support for configuring and building the TOI helper Add tests for the TOI loading/saving functions

…ing methods

iandees · 2017-04-28T02:12:37Z

This is just about done, with the exception of a couple things:

Tests are failing: It's not clear to me how the configuration tests work with an empty config. if self.yml['toi-store'] fails because of a KeyError, why doesn't self.yml['tiles']['seed'] also fail right above it? Is there a default config that's getting used somewhere?
The serializing/deserializing code assumes a tiles of interest set that contains Coordinate objects, whereas it contained coord_ints before. Since we're storing as strings anyway, I thought it'd be nicer to end up with the standard Coordinate object so we didn't have to do conversion back and forth. There might be too much work to double-check the existing assumptions (and the memory hit might be too large) to make this happen, though. (Not going to do this for now as it adds too much complexity to this PR)

zerebubuth · 2017-04-28T08:37:21Z

Is there a default config that's getting used somewhere?

Yup! There's a default config function, and the result of that is merged into the config or replaces an empty config.

That threw me, the first time I ran into it. Any way we can make it more obvious what's going on?

rmarianski · 2017-04-28T15:31:28Z

The history with the configuration is that we used to, a long time ago in a galaxy far far away, support command line switches for each of the configuration values. The idea was that you could pass in a flag for a command, and have that trump configuration values, which would trump the default settings. The "merge" idea came from combining the input from all 3 of these sources, and providing a single point of access for the rest of the code, which is what the "Configuration" object is.

Since then, we don't have command line switches for anything except where the config file is, which we could manage more directly. We could gut out most of what's there, it's just that there wasn't much motivation to do so imho. If it's confusing to maintain though, let's go ahead and do it.

iandees · 2017-04-28T15:49:00Z

Thanks for the pointers. It's likely ok as-is but I was going straight to the Configuration constructor and not seeing the call to default_yml_config().

Since this "default config" can't actually run any of the tilequeue commands (so it's more of "null object" pattern than a default), does it make more sense to adjust the Configuration constructor so that it is resilient to having missing fields in the yaml?

rmarianski · 2017-04-28T16:06:11Z

does it make more sense to adjust the Configuration constructor so that it is resilient to having missing fields in the yaml?

Sure, although what can we do, if not provide default values? If it's clearer to just inline those defaults rather than have a separate section for it, that sounds fine to me.

iandees · 2017-04-28T16:09:34Z

If it's clearer to just inline those defaults rather than have a separate section for it, that sounds fine to me

Yea, maybe that's what I was describing. Regardless, I don't think it needs to happen in this PR.

rmarianski

LGTM. 👍

Since there's new config for it, mind updating the sample config file just to see what all the possible values could be there?

rmarianski · 2017-05-01T20:44:17Z

config.yaml.sample

+  s3:
+    # The bucket and key for the s3 object that will contain the tiles of interest
+    bucket: test_bucket
+    key: toi/toi.txt.gz


👍 thanks!

I like that you namespaced the configuration based on type. I wish I had done it that way for the other configuration sections instead of flattening it out, feels cleaner.

nvkelso · 2017-05-01T22:20:03Z

tilequeue/command.py

-    """
-    logger = make_logger(cfg, 'load_tiles_of_interest')
-    logger.info('Loading tiles of interest')
+def tilequeue_dump_tiles_of_interest_from_redis(cfg, peripherals):


So this is here as a 1 time convienance for those migrating from earlier configurations, but we don't expect it to run more than once?

Right. I'll probably put together a PR after this one to remove this bit of code altogether.

Please add a comment explaining this bit (like you've done for tilequeue_load_tiles_of_interest below).

Okay, please file an issue (and the comment) so we don't loose track of it.

Fixed in d04ca1d.

iandees added 3 commits April 25, 2017 17:40

DataFetch doesn't need redis

acc94bc

Adjust tilequeue_seed to not use index_coords()

beffa75

Set TOI in pruner rather than add/remove

41e5f63

iandees added the in review label Apr 25, 2017

nvkelso reviewed Apr 26, 2017

View reviewed changes

nvkelso changed the title ~~Move tiles of interest set out of Redis and into S3~~ WIP: Move tiles of interest set out of Redis and into S3 Apr 26, 2017

iandees added 3 commits April 27, 2017 18:07

Remove reference to redis_cache_index for a more obviously-named 'toi…

390e237

…' helper Add support for configuring and building the TOI helper Add tests for the TOI loading/saving functions

Fix the logging to talk about TOI, not Redis

7a52e76

Adjust the File and S3-backed TOI helpers to use the gzipped serializ…

cf1e28d

…ing methods

Adjust default config

cac8793

iandees added 6 commits April 28, 2017 12:45

Add a command to dump the TOI from Redis to a file

83a89d7

Remove stuff related to redis_cache_index

1e26d92

Remove redis-related tests, move tile-related tests to test_tile.py

f44adff

Correct toi peripheral mock name

07e5c7f

Remove redis_cache_index maker

3a7cfce

Stick with coord_int in the in-memory toi set

ce9a9b2

iandees changed the title ~~WIP: Move tiles of interest set out of Redis and into S3~~ Move tiles of interest set out of Redis and into S3 Apr 28, 2017

iandees requested review from rmarianski and zerebubuth April 28, 2017 20:49

rmarianski approved these changes May 1, 2017

View reviewed changes

Add the TOI config to the sample config yaml

205c8ad

rmarianski reviewed May 1, 2017

View reviewed changes

nvkelso reviewed May 1, 2017

View reviewed changes

nvkelso approved these changes May 1, 2017

View reviewed changes

iandees added 4 commits May 1, 2017 18:27

Add comment to the Redis dumper command

d04ca1d

Correct the sample config header

1ab1c67

Improve redis dumping speed, add better logging

8a1aa82

Don't validate on get_key() so the key gets created if its not there

6962b49

iandees merged commit 5f5e214 into master May 1, 2017

iandees deleted the iandees/tear-the-toi-from-redis branch May 1, 2017 23:28

iandees removed the in review label May 1, 2017

This was referenced May 1, 2017

Remove Redis TOI dump command #196

Merged

Add chef support for toi-store configuration tilezen/chef-tilequeue#41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move tiles of interest set out of Redis and into S3 #193

Move tiles of interest set out of Redis and into S3 #193

iandees commented Apr 25, 2017

nvkelso Apr 26, 2017

iandees Apr 27, 2017

nvkelso commented Apr 26, 2017

iandees commented Apr 26, 2017

nvkelso commented Apr 26, 2017 •

edited

Loading

iandees commented Apr 28, 2017 •

edited

Loading

zerebubuth commented Apr 28, 2017

rmarianski commented Apr 28, 2017

iandees commented Apr 28, 2017

rmarianski commented Apr 28, 2017

iandees commented Apr 28, 2017

rmarianski left a comment

rmarianski May 1, 2017

nvkelso May 1, 2017

iandees May 1, 2017

nvkelso May 1, 2017

nvkelso May 1, 2017 •

edited

Loading

iandees May 1, 2017

Move tiles of interest set out of Redis and into S3 #193

Move tiles of interest set out of Redis and into S3 #193

Conversation

iandees commented Apr 25, 2017

nvkelso Apr 26, 2017

Choose a reason for hiding this comment

iandees Apr 27, 2017

Choose a reason for hiding this comment

nvkelso commented Apr 26, 2017

iandees commented Apr 26, 2017

nvkelso commented Apr 26, 2017 • edited Loading

iandees commented Apr 28, 2017 • edited Loading

zerebubuth commented Apr 28, 2017

rmarianski commented Apr 28, 2017

iandees commented Apr 28, 2017

rmarianski commented Apr 28, 2017

iandees commented Apr 28, 2017

rmarianski left a comment

Choose a reason for hiding this comment

rmarianski May 1, 2017

Choose a reason for hiding this comment

nvkelso May 1, 2017

Choose a reason for hiding this comment

iandees May 1, 2017

Choose a reason for hiding this comment

nvkelso May 1, 2017

Choose a reason for hiding this comment

nvkelso May 1, 2017 • edited Loading

Choose a reason for hiding this comment

iandees May 1, 2017

Choose a reason for hiding this comment

nvkelso commented Apr 26, 2017 •

edited

Loading

iandees commented Apr 28, 2017 •

edited

Loading

nvkelso May 1, 2017 •

edited

Loading