-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Redis PubSub Support #2610
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
base: development
Are you sure you want to change the base?
Redis PubSub Support #2610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem good 👍 . Didn't test yet though.
@coolwednesday the AI suggestions for the logging can be added for redacting sensitive information. Can check this in other critical flows as well.
|
Will reopen the PR after fixing the review comments, linters, tests and proper testing. |
Umang01-hash
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can we rename the file
metrics_interface.goinside redis directory tomock_metrics.go. - We have a file
config_test.gobut in respect to thatconfig.gofile doesn;t exist. That violates Go naming conventions and makes code harder to discover. - The file
pubsub_query_test.gocontains only 2 tests for theQuery()method, butpubsub_test.goalready has query tests (testChannelQuery()andtestStreamQuery()ingetQueryTestCases()). We can deletepubsub_query_test.goand merge the 2 tests intopubsub_test.go.
| "gofr.dev/pkg/gofr/datasource/pubsub/kafka" | ||
| "gofr.dev/pkg/gofr/datasource/pubsub/mqtt" | ||
| "gofr.dev/pkg/gofr/datasource/redis" | ||
| gofrRedis "gofr.dev/pkg/gofr/datasource/redis" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for adding this alias??
| // Use the embedded PubSub from the Redis client | ||
| if c.Redis != nil { | ||
| // Type assert to access PubSub field | ||
| if redisClient, ok := c.Redis.(*gofrRedis.Redis); ok && redisClient != nil && redisClient.PubSub != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using type assertion for initilalizing any other PubSub client then why do it for redis as it violates the existing architectural pattern. Maybe we can create separate redis.NewPubSub() constructor that returns pubsub.Client directly.
| } | ||
| } | ||
|
|
||
| func TestGetRedisConfig_PubSubStreams_InvalidValues(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A common thing that all test methods have is they need a mockLogger and mockConfig, so can we add a testGetRedisConfig(t, configMap) helper function that can take the map and directly return us *Config from getRedisConfig
| ) | ||
|
|
||
| const ( | ||
| modePubSub = "pubsub" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where this constant is being used??
| } | ||
|
|
||
| func (m *streamMessage) Commit() { | ||
| err := m.client.XAck(context.Background(), m.stream, m.group, m.id).Err() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using context.Background() here could cause the Commit operation to hang indefinitely if Redis is slow or unresponsive, consider adding a timeout context to prevent goroutine hangs.
|
|
||
| // PubSub constants. | ||
| defaultRetryTimeout = 10 * time.Second | ||
| messageBufferSize = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message buffer size of 100 is too small for production workloads and will lead to frequent message drops under moderate load. Consider a typical scenario:
- Message rate: 500 msg/sec per topic
- Consumer processing time: 50ms per message
- Buffer fills in: 0.2 seconds
|
|
||
| hostname, _ := os.Hostname() | ||
|
|
||
| return fmt.Sprintf("consumer-%s-%d", hostname, time.Now().UnixNano()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consumer name generation using time.Now().UnixNano() can produce duplicate values when called in rapid succession (within the same nanosecond), leading to consumer ID collisions in Redis Streams consumer groups.
| } | ||
|
|
||
| if ps.parent.logger != nil { | ||
| traceID := span.SpanContext().TraceID().String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
traceID is calculated but never used (it's passed to logPubSub() via span, not directly).
| } | ||
|
|
||
| addr := fmt.Sprintf("%s:%d", ps.parent.config.HostName, ps.parent.config.Port) | ||
| res.Details["addr"] = sanitizeRedisAddr(addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sanitizeRedisAddr() function is designed to remove credentials from Redis connection URLs (e.g., redis://user:pass@host:port), but here we're passing a simple host:port string that never contains credentials.
| require.NoError(t, err) | ||
|
|
||
| // Miniredis compatibility check | ||
| if len(res) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not have if else conditions in test? Please remvoe it and separate it to two test methods if requried.
Redis PubSub: Streams Support and Default Mode Change
Description:
pubsubtostreams. The implementation follows the same patterns as other pubsub providers (NATS, EventHub, Kafka) and includes full support for consumer groups, acknowledgments, and message persistence.Testing Redis PubSub
Prerequisites
Start Redis using Docker:
Testing Streams Mode (Persistence) - Default
Streams mode provides persistent messaging with consumer groups and acknowledgments. Messages are stored in Redis and can be processed even after app restarts.
Configuration
Create
configs/.env:Test Persistence
Start Publisher (using
gofr/examples/using-publisher):cd gofr/examples/using-publisher go run main.goPublish messages:
Start Subscriber (using
gofr/examples/using-subscriber):cd gofr/examples/using-subscriber go run main.goVerify Persistence:
Testing PubSub Mode (Fire-and-Forget)
PubSub mode provides non-persistent messaging. Messages are delivered only to active subscribers and are lost if no subscriber is listening.
Configuration
Create
configs/.env:Test Fire-and-Forget Behavior
Start Subscriber first:
cd gofr/examples/using-subscriber go run main.goPublish messages (subscriber is listening):
Stop the subscriber and publish more messages:
Restart subscriber:
Features
✅ Streams Mode (Default)
✅ PubSub Mode
Checklist:
goimportandgolangci-lint.