Design review on redis/rediscluster store

[hsinhoyeh]

This is a following up discussion on the design review of redis driver. the original feature requirements are discussed here: https://github.com/docker/libkv/issues/9 For store interface, please refer to here: https://github.com/docker/libkv/blob/master/store/store.go#L63

Redis is an in-memory key/value storage, a single thread server which supports rich data structures and lua script. It also can grant ttl(time-to-live) for each key and evict the expired keys automatically. This feature enables us to impl such functions straightforward.

Put(key string, value []byte, options *WriteOptions) error
Get(key string) (*KVPair, error)
Delete(key string) error
Exists(key string) (bool, error)

Redis also provided a scan feature which lookups all keyspace with a pattern given. It can be used to impl List and DeleteTree methods with the following file hierarchy:

set /foo bar
set /dir1/foo bar
set /dir1/dir2/foo bar

so if we call List("/"), we need to scan all keyspace and return those keys matched "/". if we call List("/dir1/dir2"), we need to use pattern "/dir1/dir2/" instead. In this case, DeleteTree will be performed in two steps: 1. list the tree 2. batch delete all keys in the tree. But that really depends how atomic we want here. If we need this operation atomaitcally, we need to move these two functions into a lua script (which will be discussed later)

For Lock implementation, redis did provide such features called "set if not exist" and "set if exist" http://redis.io/commands/set so the one who create the key owns the lock. Release the lock actually means delete the key. The impl can be trivial as well (for handling ttl, we just need a goroutine to refresh it's expiration time through calling set)

redisclient.Do("set", $key, $value, "NX", "EX", $ttl_in_second)
// once we hold the key, we can have a dedlicated goroutine to handle ttl
ticker := time.NewTick( ttl / 3)
for range ticker.T {
    redisclient.Do("set", $key, $value, "XX", "EX", $ttl_in_second) // update ttl only when key exist.
}

Watch API basically allows client to receive events regarding to the changes of a key (or a directory). In redis, we can borrow a keyspace notification feature(http://redis.io/topics/notifications). Since keyspace notification will deliver any events of the whole entire keyspace, client need to filter out thoese irrelevants.

func (r *Redis) WatchXXX(key string, stopCh <-chan struct{}) (<-chan *KVPair, error){
    psc := redislib.PubSubConn{client: r.client}
    psc.PSubscribe("__keyevent*__:*") // doing a pattern subscribe for all keyevent notification
    respChan :=make(chan *KVPair)
    go func(){
        for {
            // watch stopChan
            select{
                case <-stopCh:
                    // doing unsubscribe and close respChan..
                default:
            }
            switch n := psc.Receive().(type) {
                    case redis.PMessage:
                        // filter the key part and enque to respChan if found any of interest

            }
        }
    }()
    return respChan, nil
}

Script allows multiple non-blocking commands to be run without being interrupted by any coming client requests. Here is a simple script example from stackoverflow So that is really useful to impl DeleteTree, AtomicPut and AtomicDelete.

Feel free to modify this issue and please let me know how you think about this. thanks

[abronan]

@hsinhoyeh Thanks, it's a great proposal.

About the lock it's actually the same implementation than etcd's lock so the code will be quite trivial as it will follow the same pattern. (even though apparently, the native go client for redis supports Distributed Locking out of the box)

As for the lua scripts I'm not familiar enough with redis to see how this could help in the case of DeleteTree/AtomicPut or AtomicDelete. Would be great not to go outside of what the native go client for redis is offering. If this is indeed an issue, we can see how to use eval later on.

Also there is the remaining question of which one of the existing redis clients for golang is going to be used.

Once we know the one we'll use (at a first glance, I would lean towards go-redis/redis), I think there are enough elements in this proposal to put up a PR together. Most of the tradeoffs have been identified with this proposal and we can try to fit the redis backend onto the actual set of tests.

/cc @runcom (cc'ing you as you were the author of the initial Proposal, let us know what you think and if there is any detail missing)

[Akagi201]

@hsinhoyeh How about this redis driver? https://github.com/mediocregopher/radix.v2 I use this driver a lot on my projects. The code structure is better than redis.v3

[hsinhoyeh]

sorry for my late response, I will send a PR for this design doc soon.

@Akagi201 thanks for the input. I am open to any redis drivers.

[hsinhoyeh]

hi guys so sorry for the delay. I have drafted out implementation for this proposal. please leave comments and suggestions, thanks

@abronan @runcom @Akagi201

[thibaultmeyer]

Hi @abronan @runcom @Akagi201 @hsinhoyeh

When this feature will be ready ? (Traefik use this lib for the Key/Value store) and it could be interresting to have Redis as Key/Value store rather than Consul or other "big monster" alternatives.

Redis is very smart and simple to install / configure

[hsinhoyeh]

@0xbaadf00d It has been a while that this repo maintainers are not show up for reviewing and commenting. any suggestions?

[thibaultmeyer]

@hsinhoyeh Nice implementation, Is it possible to set the database number to use ? https://redis.io/commands/select

[abronan]

Hi @0xbaadf00d and @hsinhoyeh,

This repository seems abandoned for now (I was one of the maintainers). In an attempt to get it back at a "working" state I merged some of the contributions and patches (with the redis store) onto my fork for now. I plan to carry more patches that were submitted here and I'm almost done with a new etcdv3 client implementation (which should fix a few issues encountered on traefik as well). Not sure people will switch to the fork to be honest, not a fan of maintaining a fork either, but I'll maintain it on my side. Feel free to submit anything onto this fork and I'll take the time to review and merge.

[hsinhoyeh]

@0xbaadf00d This patch has not addressed this issue yet. But it could work on single instance scenario, not in redis cluster. Need to work on it though.

@abronan thanks your effort to take the responsibility. Let's keep it working