Adding Support for Redis Cluster Client #46
Conversation
…dis-cluster-client-support
…dis-cluster-client-support
|
No idea why it says the tests fail, as I can see in Actions that they pass I have updated the version to 0.6.1, so we can release once this is merged to main. |
|
Looking for re-review: I updated a log line with the previous state to fulfill this request: #45 |
cmd/main.go
Outdated
| var redisUrl = flag.String("redis", "", "URI for the Redis cluster (host:port)") | ||
| var redisUrl = flag.String("redis", "", "For single node redis instances: URI "+ | ||
| "for the Redis instance (host:port). For redis clusters: a comma-separated list of redis nodes. "+ | ||
| "If using an ElastiCache redis cluster with cluster mode enabled, you can supply the configuration endpoint.") |
There was a problem hiding this comment.
| "If using an ElastiCache redis cluster with cluster mode enabled, you can supply the configuration endpoint.") | |
| "If using an ElastiCache Redis cluster with cluster mode enabled, you can supply the configuration endpoint.") |
Does it apply on any Redis deployment in cluster mode? or just Elasticache? If any, then we can remove the "Elasticache" part
pubsub/listener.go
Outdated
| listener.logger.Info("Event `%s` transitioned FSM [%s] to state `%s` from state `%s` - updating store", | ||
| request.Event.Transition.Event, smId, fsm.State, previousState) |
There was a problem hiding this comment.
Could we add a log line before we try to "SendEvent" ? It seems like it can fail there and we wouldn't have the IDs logged (SM ID, event ID, previous state)
There was a problem hiding this comment.
Sure, adding as a debug entry
listener.logger.Debug("Preparing to send event `%s` for FSM [%s] (current state: %s)",
request.Event.Transition.Event, smId, previousState)
There was a problem hiding this comment.
not only we capture the error if SendEvent fails, but we also send a notification (L98)
…/z-cran/go-statemachine into add-redis-cluster-client-support
pubsub/listener.go
Outdated
| listener.logger.Info("Event `%s` transitioned FSM [%s] to state `%s` from state `%s` - updating store", | ||
| request.Event.Transition.Event, smId, fsm.State, previousState) |
There was a problem hiding this comment.
not only we capture the error if SendEvent fails, but we also send a notification (L98)
pubsub/listener.go
Outdated
| @@ -101,8 +102,8 @@ func (listener *EventsListener) ListenForMessages() { | |||
| request.GetEvent().GetTransition().GetEvent(), err))) | |||
| continue | |||
| } | |||
| listener.logger.Info("Event `%s` transitioned FSM [%s] to state `%s` - updating store", | |||
| request.Event.Transition.Event, smId, fsm.State) | |||
| listener.logger.Info("Event `%s` transitioned FSM [%s] to state `%s` from state `%s` - updating store", | |||
There was a problem hiding this comment.
please use Debug not Info - this gets really noisy quickly (I know I used INFO before, my bad)
| @@ -150,6 +150,11 @@ func NewRedisStore(address string, isCluster bool, db int, timeout time.Duration | |||
| var tlsConfig *tls.Config | |||
| var client redis.Cmdable | |||
|
|
|||
| if os.Getenv("REDIS_TLS") != "" { | |||
There was a problem hiding this comment.
this is already in main, I believe?
can you please do:
- update your
mainbranch to be in sync with this repo'smain - rebase your branch
add-redis-clusteron yourmainand resolve conflicts (if any) - make sure you run tests (
make test) and they all pass
thanks.
There was a problem hiding this comment.
yep, see here it's been merged in main and release and it's in Release 0.6.0
|
Closing, had some weird issues from the rebase between upstream & current branch. Wasn't able to open a branch & normal PR on main repo so I made a fork originally. New PR with these change: #50 |
Summary of Changes
redis.ClusterClientorredis.Clientin theStoreManager-clusterflag to toggle whether to use the cluster client or whether to use the standard client