search

usmanm / redis-tdigest

t-digest module for Redis
http://redismodules.com/modules/redis-tdigest/
MIT License
72 stars 12 forks source link

fix aof output support #8

Closed bpo closed 3 years ago

bpo commented 3 years ago

This module was expecting to use EmitAOF's fmt string like sprintf, but redis modules have their own string format (see moduleCreateArgvFromUserFormat in the Redis source).

Notably there is no built-in support for writing doubles, so this change dumps double values to a buffer before the EmitAOF calls.

bpo commented 3 years ago

Just for further context, prior to this patch if you have this module enabled on a Redis database that is using AOF, AOF rewrites will fail with:

Fatal: AOF method for module data type 't-digest0' tried to call RedisModule_EmitAOF() with wrong format specifiers '%s %ll'
usmanm commented 3 years ago

Thanks for the patch @bpo! Is it possible to add a test case for this?

bpo commented 3 years ago

@usmanm The existing AOF code is unexercised by tests, so it's nontrivial. But yes- assertions against the resulting AOF file would probably be the best approach. Simply grepping for the error string in the logs would not be robust against changes in Redis.

usmanm commented 3 years ago

Ah, I meant would you be able to add a test for this? All good if not, I can look into it later.

bpo commented 3 years ago

I should be able to add tests sometime soon, was just looking for feedback on the approach.

bpo commented 3 years ago

@usmanm I added a test for successful AOF rewrites, and another one for RDB just to be complete. The tests now bounce the redis-server and check to see if a tdigest key still exists.