Feature/delete service stats worker jobs

[eguzki]

• Stats::PartitionEraserJob
  Background job to delete stat keys

• Stats::PartitionGeneratorJob
  Background job to generate Stats::PartitionEraserJob for each stats keys subset

[davidor]

There are mainly 2 aspects of this PR that I think should be addressed before merging.

The first one is that there are no tests. The second one is that, in my opinion, the responsibilities of the 2 types of jobs (PartitionGeneratorJob and PartitionEraserJob) are not well defined and they do a lot of duplicated work.

PartitionGeneratorJob generates all the keys, but it discards them and only passes to each of the PartitionEraserJobs an offset. Then, each PartitionEraserJob generates again all the keys and discards all of them except a few ones (just picks N starting from the 'offset' received). It's a lot of duplicated work.

Wouldn't it be better to generate all the keys in the Generator job and just pass X of them to each of the Delete jobs? That way, the jobs would not perform any duplicated work. Also, I think that conceptually it would be easier to understand, because one job type would be responsible for generating all the keys, and the other just for deleting the keys that it received.

[eguzki]

The design was made to avoid serializing/deserializing to/from redis all the keys that have to be deleted. You are proposing to do that.

Besides, resque job size (basically json body size) can be big. The body size is controlled by PARTITION_BATCH_SIZE configuration parameter. Having PARTITION_BATCH_SIZE ~ 1000 keys, it makes ~ 1000 * (50 bytes per key) = 50k the size of a PartitionEraserJob resque job.

The trade off is

• small PARTITION_BATCH_SIZE => small job body and lots of PartitionEraserJob jobs
• big PARTITION_BATCH_SIZE => big job body and few PartitionEraserJob jobs. If some job fail, a new worker will be processed and the big partition will be erased again (many keys could already been deleted)

Other implementation would be passing indexes besides jobs, and eraser worker will only have to generate required keys, hence no duplicated work. Resque jobs would be small in size and no keys would be serialized to redis. However the implementation of indexes increases complexity of the key generator and I decided to leave it for now. Maybe future improvement.

WDYT @davidor ? Can we afford big jobs and serializing all the keys including them in job bodies?

[eguzki]

Implementation of PARTITION_BATCH_SIZE and DELETE_BATCH_SIZE as configuration params with default values

[davidor]

Serializing/deserializing could be a problem too. This is the kind of thing that needs to be measured.

What if Stats::PartitionGeneratorJob generated to and from timestamps instead of indexes? Would that be a valid option? That way each job would be responsible for generating and deleting a part of the keys without duplication and without needing to serialize/deserialize the keys.

For example, if we need to delete a whole year of stats, we could generate one job for each month, or for each day. Not sure what would be the most appropriate granularity. Or maybe split by apps or metrics?

[eguzki]

Partitioning by time, applications, metrics or users is rough way of partitioning. You cannot control the number of keys received by a worker to be deleted. With the current implementation, the size of a partition is controlled by a configuration parameter.

Anyway, it is a partitioning, yes.

We could leave it like it is now and write an issue to implement indexes. WDYT @davidor ?

[eguzki]

@davidor ready for a review

[davidor]

I'm still not convinced about the idea of generating the whole set of keys on every PartitionEraserJob. It'd be good to hear other opinions on this. @unleashed ? @miguelsorianod ?

This might be good enough for a first version but we'll need to closely measure the impact on CPU time consumed by this kind of worker with something like stackprof. It's difficult to tell now because there are many factors that can have an impact like number of this kind of jobs generated, the average from-to interval, etc.

At least we should add a TODO here explaining what this does and mention that this is probably something that could be improved.

[miguelsorianod]

isn't just a subset of the keys generated/removed (length) ?

[eguzki]

it is generating all keys up to offset + length.

I will write an issue about improving the algorithm to use indexes.

Deleting service stats is not a job that will be created often

[davidor]

Only a subset is deleted, but all of them are generated stats_key_gen.keys.

[miguelsorianod]

👍 , I was thinking on the old implementation, when enumerators were used and not all keys were generated.

[davidor]

Good job @eguzki 👍
I think this can be merged to the integration branch.