Skip to content

Conversation

@Adrian2112
Copy link

I was testing the distributed redis with an app that I have and noticed the mget and mset didn't work, obviously because you do not know in which node this should be set. Then I saw the impementation for Redis::Distributed#del and thought we can do something similar for this.
I'm not sure if you want this or it is intentionally left out so anybody do this on their own if needed.
Either way here it is.

@yaauie
Copy link
Contributor

yaauie commented Nov 13, 2014

This adapter gem aims to be as transparent as possible, without adding unnecessary complexity or creating new forms of failure; supporting mset in distributed mode, as above, creates a new failure scenario, in which a single ruby command can cause partial failure (e.g., data is written on one or more nodes, followed by an attempt to write to a down node, resulting in a partial write).

As such, I am not convinced that this patch should be accepted (despite the prior art of del behaving in a similar manner), although I am open to discussion.

@yaauie
Copy link
Contributor

yaauie commented Nov 13, 2014

Unfortunately, the redis-rb adapter gem v3.x still supports 1.8.7, which doesn't have Enumerable#each_with_object, so the build breaks 😦

 1) Error:
test_mget(TestDistributedCommandsOnStrings):
NoMethodError: undefined method `each_with_object' for #<Hash:0x7f31cc7bef08>
/home/travis/build/redis/redis-rb/lib/redis/distributed.rb:293:in `mapped_mget'
/home/travis/build/redis/redis-rb/lib/redis/distributed.rb:286:in `mget'
/home/travis/build/redis/redis-rb/test/distributed_commands_on_strings_test.rb:15:in `test_mget'
/home/travis/build/redis/redis-rb/test/helper.rb:70:in `run'

2) Error:
test_mget_mapped(TestDistributedCommandsOnStrings):
NoMethodError: undefined method `each_with_object' for #<Hash:0x7f31cc777310>
/home/travis/build/redis/redis-rb/lib/redis/distributed.rb:293:in `mapped_mget'
/home/travis/build/redis/redis-rb/test/distributed_commands_on_strings_test.rb:23:in `test_mget_mapped'
/home/travis/build/redis/redis-rb/test/helper.rb:70:in `run'

@Adrian2112
Copy link
Author

Right, I fixed that. Changed each_with_object to inject.
Regarding the failure scenario, I agree that there could be a problem similar to what you said. But I think it would be good to have something so this commands can be used transparently form a redis client regardless if the client is one server or a distributed one.
Do you think that this should belong in another gem that handles said problems?

@djanowski
Copy link
Collaborator

@Adrian2112 I agree with @yaauie, having del behave that way is not something desirable. In any case, Redis::Distributed will be split into a separate gem soon and we can revisit how we want it to operate. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants