Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added single Redis instance Redlock support. #32

Closed
wants to merge 6 commits into from

Conversation

wingunder
Copy link
Contributor

The definition of the Redlock (template) class is available in
src/sw/redis++/redlock.*

The tests are implemented in test/src/sw/redis++/redlock_test.h*

The implementation is meant for a single Redis instance or for a
single clustered Redis instance. The locking of multiple instances is
not supported, but could be added.

The definition of the Redlock (template) class is available in
src/sw/redis++/redlock.*

The tests are implemented in test/src/sw/redis++/redlock_test.h*

The implementation is meant for a single Redis instance or for a
single clustered Redis instance. The locking of multiple instances is
not supported, but could be added.
Thanks to sewenew for pointing this out.

Another test case was added to cover this situation.
The problem is that Redis is not really spot on with removing timed
out keys. This means that a key might still be around a few
milliseconds after it should have expired.

The test TTL is now 200ms.
Thanks to sewenew for giving me this idea.
We now allow 2 ms for latency:
   - 1 ms for extend_lock()
   - 1 ms for Redis expire latency.
We now just call 'EVALSHA' and if it fails in cluster mode, we assume
that the key moved to a node that has not got the script loaded
yet. In this case we simply load the script and call 'EVALSHA' again.

#include "redis++.h"
#include <unistd.h>
#include <openssl/rc4.h>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you don't use openssl stuff for the implementation of Redlock, you should not include this header. Otherwise, if the file is missing, i.e. openssl is not installed, your code won't compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This was a silly mistake from my side. The openssl.h include is now only in the test cases. See PR #33.


namespace redis {

class RandomBufferInterface
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use @antirez's method to generate random strings automatically.

Copy link
Contributor Author

@wingunder wingunder Nov 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it should be up to the user to decide if they want more speed or more randomness. The RandomBufferInterface's implementation is now moved to the test cases. See PR #33.

Redlock(RedisInstance& instance, RandomBufferInterface& randomBuffer) :
_randomBuffer(randomBuffer),
_instance(instance),
_unlockScript("\
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try a new C++ feature: raw string literal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean _unlockScript(R"...")?
If so, I could not get this right (lack of C++ skills), so could you please show me how to do it in RedLock::RedLock() in redlock.cpp of PR #33?

Copy link
Owner

@sewenew sewenew Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try this: _unlockScript(R"(raw_characters )")

const std::string _extendLockScript;
const std::string _unlockSHA1;
const std::string _extendLockSHA1;
std::map<std::string, std::string> _randomNumberMap;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you don't need the keys to be ordered, unordered_map might be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, you're right. I however stopped tracking the locks in PR #33.

virtual std::string getUpdatedString() = 0;
};

template <typename RedisInstance>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to be a template. In fact, the original RedLock algorithm only try to solve the distributed lock problem with a single Redis instance or multiple independent Redis instances. If you do need to lock with Redis master. You can ask the user to get the Redis object with RedisCluster::redis(hash_tag) method, and pass it to Redlock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, you're right. I now have "RedLock" as a standard class in PR #33.

@wingunder
Copy link
Contributor Author

I'm closing this PR, as it bases on master. @sewenew is working on the redlock implementation on the recipes branch, so I took all of the good stuff from this PR and put it into PR #33, which is based on the recipes branch.

@wingunder wingunder closed this Nov 4, 2019
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.

2 participants