added redis storage option #96
Conversation
|
Really nice contribution! Thanks for taking the time. I have a couple of comments:
And I saw that you have added a retention to the entries in Redis. What is the intention of expiring the entries? Is this to be used in combination with a retention in Kafka too? I think one should have the option of setting no retention (it seems to be mandatory now). |
storage/redis/builders.go
Outdated
| func RedisBuilder(client *redis.Client, namespace string) storage.Builder { | ||
| return func(topic string, partition int32) (storage.Storage, error) { | ||
| if namespace == "" { | ||
| namespace = strconv.FormatInt(time.Now().UnixNano(), 10) |
There was a problem hiding this comment.
It would be better to return an error here and make the namespace a required parameter. Using the time as the namespace would mean that the application has to recover on every launch and leave a lot of obsolete data from previous runs in Redis.
storage/redis/redis.go
Outdated
| } | ||
|
|
||
| // NewRedis creates a new Storage backed by Redis. | ||
| func NewRedis(client *redis.Client, hash string) (storage.Storage, error) { |
There was a problem hiding this comment.
I would remove the Redis suffixes and prefixes from the types and methods as the package path and name implies this is a Redis storage for Goka. I think it would be cleaner to have redis.New.
storage/redis/redis.go
Outdated
|
|
||
| // Close removes all data present current hash(`namespace:table:partition`) | ||
| // instead closing the redis client. | ||
| func (s *redisStorage) Close() error { |
There was a problem hiding this comment.
Close should close the client and leave the data in Redis. Deleting the data on close would force the applications to recover their state on every launch and this is not in line with the other storage implementations. Functionality for cleaning the state from Redis would definitely be a convenient addition but I would move it to a separate method.
There was a problem hiding this comment.
I don't know why it is deleting the data, but I don't think that Close should actually close the client too. The caller should care about closing the client since the Redis client is passed to the storage builder and all partitions share the same client.
There was a problem hiding this comment.
I completely overlooked that part. Even if it wasn't shared with other builders, it would be better to leave it to caller who built the client.
There was a problem hiding this comment.
I put this delete here, 'cause I thought that when you close a partition, you would not use it anymore by this consumer. But probably I thought on the wrong direction.
heltonmarx
force-pushed
the
redis-storage
branch
from
32fe4a4 to
be3a119
Compare
Inserted as a storage option the use of Redis, passing through Builder the redis client.
Please, any suggestions will always be welcome.