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.

Sign up for GitHub

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

New Wrapper for cluster as in ISSUE #113 #128

Closed
wants to merge 3 commits into from
Closed

New Wrapper for cluster as in ISSUE #113 #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b857f9f
fix using another wrapper to fit redis cluster and redis single node;
Sep 26, 2022
811faa3
update test files with new openConn;
Sep 26, 2022
3113601
fix addr to local
Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cleaner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import (
)

func TestCleaner(t *testing.T) {
flushConn, err := OpenConnection("cleaner-flush", "tcp", "localhost:6379", 1, nil)
flushConn, err := OpenConnection("cleaner-flush", sOp, nil)
assert.NoError(t, err)
assert.NoError(t, flushConn.stopHeartbeat())
assert.NoError(t, flushConn.flushDb())

conn, err := OpenConnection("cleaner-conn1", "tcp", "localhost:6379", 1, nil)
conn, err := OpenConnection("cleaner-conn1", sOp, nil)
assert.NoError(t, err)
queues, err := conn.GetOpenQueues()
assert.NoError(t, err)
Expand Down Expand Up @@ -75,7 +75,7 @@ func TestCleaner(t *testing.T) {
assert.NoError(t, conn.stopHeartbeat())
time.Sleep(time.Millisecond)

conn, err = OpenConnection("cleaner-conn1", "tcp", "localhost:6379", 1, nil)
conn, err = OpenConnection("cleaner-conn1", sOp, nil)
assert.NoError(t, err)
queue, err = conn.OpenQueue("q1")
assert.NoError(t, err)
Expand Down Expand Up @@ -122,7 +122,7 @@ func TestCleaner(t *testing.T) {
assert.NoError(t, conn.stopHeartbeat())
time.Sleep(time.Millisecond)

cleanerConn, err := OpenConnection("cleaner-conn", "tcp", "localhost:6379", 1, nil)
cleanerConn, err := OpenConnection("cleaner-conn", sOp, nil)
assert.NoError(t, err)
cleaner := NewCleaner(cleanerConn)
returned, err := cleaner.Clean()
Expand All @@ -133,7 +133,7 @@ func TestCleaner(t *testing.T) {
assert.NoError(t, err)
assert.Len(t, queues, 2)

conn, err = OpenConnection("cleaner-conn1", "tcp", "localhost:6379", 1, nil)
conn, err = OpenConnection("cleaner-conn1", sOp, nil)
assert.NoError(t, err)
queue, err = conn.OpenQueue("q1")
assert.NoError(t, err)
Expand Down
15 changes: 8 additions & 7 deletions connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,15 @@ type redisConnection struct {
}

// OpenConnection opens and returns a new connection
func OpenConnection(tag string, network string, address string, db int, errChan chan<- error) (Connection, error) {
redisClient := redis.NewClient(&redis.Options{Network: network, Addr: address, DB: db})
return OpenConnectionWithRedisClient(tag, redisClient, errChan)
func OpenConnection(tag string, redisOption *redis.Options, errChan chan<- error) (Connection, error) {
// redisClient := redis.NewClient(&redis.Options{Network: network, Addr: address, DB: db})
redisClient := redis.NewClient(redisOption)
return OpenConnectionWithRmqRedisClient(tag, RedisSingleWrapper{redisClient}, errChan)
}

// OpenConnectionWithRedisClient opens and returns a new connection
func OpenConnectionWithRedisClient(tag string, redisClient redis.Cmdable, errChan chan<- error) (Connection, error) {
return OpenConnectionWithRmqRedisClient(tag, RedisWrapper{redisClient}, errChan)
// OpenConnectionWithRedisClusterClient opens and returns a new connection for a cluster redis
func OpenConnectionWithRedisClusterClient(tag string, clusterClient *redis.ClusterClient, errChan chan<- error) (Connection, error) {
return OpenConnectionWithRmqRedisClient(tag, RedisClusterWrapper{clusterClient}, errChan)
Copy link
Owner

Choose a reason for hiding this comment

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

Have you tried using the existing wrapper with the cluster client?

Suggested change
return OpenConnectionWithRmqRedisClient(tag, RedisClusterWrapper{clusterClient}, errChan)
return OpenConnectionWithRmqRedisClient(tag, RedisSingleWrapper{clusterClient}, errChan)

When I compare these files
https://github.com/adjust/rmq/blob/3113601ad47836e110cd3201f160b69c172f9770/redis_wrapper_single.go
https://github.com/adjust/rmq/blob/3113601ad47836e110cd3201f160b69c172f9770/redis_wrapper_cluster.go
it looks like the cluster client has exactly the same functions, so I wonder if it might actually implement the redis.Cmdable interface, in which case we should be able to still use the existing wrapper instead of having the need for two different (but almost identical) wrapper implementations.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I have tried, works fine with me 💯

Copy link
Author

Choose a reason for hiding this comment

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

so I wonder if it might actually implement the redis.Cmdable interface

This also confuses me, the redis.Client does implement the redis.Cmdable, but, redis.ClusterClient does not.

I have checked the wrapper functions, which is common, and my team also wrapped same functions both for cluster and single redis.

It also needs some work to do to make the code more elegent, such as making new interface to fit the two type of redis clients, I will see what I can do in the future, thanks for pointing out! 👍

Copy link

Choose a reason for hiding this comment

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

The redis.ClusterClient has implementd Cmdable already, so this wrapper might be not necessary

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for jumping in here! 🙌

Do you think you can take this branch and remove this wrapper again to see if it works without it? Feel free to open a new PR with the updated branch then. Thanks again!

}

// OpenConnectionWithTestRedisClient opens and returns a new connection which
Expand All @@ -77,7 +78,7 @@ func OpenConnectionWithTestRedisClient(tag string, errChan chan<- error) (Connec
return OpenConnectionWithRmqRedisClient(tag, NewTestRedisClient(), errChan)
}

// If you would like to use a redis client other than the ones supported in the constructors above, you can implement
// OpenConnectionWithRmqRedisClient If you would like to use a redis client other than the ones supported in the constructors above, you can implement
// the RedisClient interface yourself
func OpenConnectionWithRmqRedisClient(tag string, redisClient RedisClient, errChan chan<- error) (Connection, error) {
name := fmt.Sprintf("%s-%s", tag, RandomString(6))
Expand Down
Loading