redis queue: fix loading old jobs from the database #27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The old code wasn't able to restore the jobs stored into a redis
database because of different issues:
* The code to find all the gearman-related keys into Redis was wrong,
it didn't use globbing, causing zero keys to be found.
* Each key is made by three parts: gearman identifier (never changes),
name of the function and unique id. The old code wasn't able to
associate the correct value to the function and the unique id becase
the scan code was using a wrong pattern (the `-` was not ignored).
* The code stored the keys as binary data instead of strings. That
caused the keys to have null terminators as part of their name,
hence making the retrieval of the job data fail (an empty data was
always returned).
* The code created the job representation in memory without making a
duplicate of the data attribute. The char* holding the data was
freed by the hiredis code and later, as soon as a worker tried to
access it caused a segmentation fault on the server side.
With these changes everything is working as expected.
Signed-off-by: Flavio Castelli <fcastelli@suse.com>
SpamapS
reviewed
Oct 20, 2016
| @@ -202,7 +202,7 @@ static gearmand_error_t _hiredis_add(gearman_server_st *, void *context, | |||
| build_key(key, unique, unique_size, function_name, function_name_size); | |||
| gearmand_log_debug(GEARMAN_DEFAULT_LOG_PARAM, "hires key: %u", (uint32_t)key.size()); | |||
|
|
|||
| redisReply *reply= (redisReply*)redisCommand(queue->redis(), "SET %b %b", &key[0], key.size(), data, data_size); | |||
| redisReply *reply= (redisReply*)redisCommand(queue->redis(), "SET %s %b", &key[0], data, data_size); | |||
There was a problem hiding this comment.
I'm a wee bit concerned that we're trading a nice safe fixed length string for a hopefully null terminated string. Is there some reason for that?
There was a problem hiding this comment.
I had to change that because this GET request would have always returned an empty result. The key searched would not have been null terminated. We could change that to GET %b...
Also inside of Redis the key would be stored as _gear_-foo-123\00\00\00\00 which is not so nice :)
We are in full control over the key generation, hence there should not be relatively safe
There was a problem hiding this comment.
ah Ok, I didn't really grasp what the vector had in it. Makes sense.
esabol
pushed a commit
to esabol/gearmand
that referenced
this pull request
Mar 23, 2017
redis queue: fix loading old jobs from the database
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old code wasn't able to restore the jobs stored into a redis
database because of different issues:
-was not ignored).always returned).
With these changes everything is working as expected.
Signed-off-by: Flavio Castelli fcastelli@suse.com