[FSSDK-12072] Integrate Redis Streams into agent #448
Conversation
This commit integrates Redis Streams for persistent notification delivery from v4.3.0-beta.1 with the existing CMAB Redis cache support in master. Features Added: - Redis Streams implementation for notification synchronization with persistent message delivery, automatic retries, consumer groups, and configurable batching - Flexible Redis authentication utility supporting auth_token, redis_secret, and password fields with environment variable fallback - Applied flexible auth pattern to all Redis clients: ODP cache, UPS, CMAB cache, and synchronization pubsub - Configurable CMAB prediction endpoint via config or environment variable Changes: - Added pkg/syncer/pubsub/redis_streams.go and comprehensive tests - Added pkg/utils/redisauth for flexible password authentication - Updated all Redis clients (ODP, UPS, CMAB) with UnmarshalJSON for auth - Updated config.yaml with Redis Streams parameters and auth_token field - Updated CHANGELOG.md with [Unreleased] section documenting new features - Updated CI workflow to use Redis 6 and added Redis to test-coverage job - Added docs/redis-streams.md with complete Redis Streams documentation Configuration: - CMAB configuration remains under client.cmab section - Redis Streams enabled via synchronization.notification.default - All Redis clients support auth_token, redis_secret, password fields - Environment variables: REDIS_PASSWORD, REDIS_ODP_PASSWORD, REDIS_UPS_PASSWORD, REDIS_CMAB_PASSWORD Testing: - All existing tests pass - Added 5 new auth tests for CMAB Redis cache - Redis Streams includes 18 comprehensive tests - redisauth utility includes 14 tests covering all scenarios Both features coexist without conflicts - Redis Streams uses STREAM data type for messaging while CMAB cache uses STRING type for key-value storage.
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
Looks great! A few suggestions to be considered.
| enable: true | ||
| ## Use "redis" for fire-and-forget pub/sub (existing behavior) | ||
| ## Use "redis-streams" for persistent message delivery with retries and acknowledgment | ||
| default: "redis-streams" |
There was a problem hiding this comment.
Do clients need any changes for redis vs redis-streams?
Clients with redis notifications can switch to redis-streams without any change in the client server?
There was a problem hiding this comment.
What is downside of redis-streams? Wondering if we can support redis-streams only (batch=1 same as the current redis option?)
| } | ||
|
|
||
| // First, unmarshal normally to get all fields | ||
| if err := json.Unmarshal(data, alias); err != nil { |
There was a problem hiding this comment.
I do not see alias used any where. Can't it be removed? Wondering if you have other reason for this.
| } | ||
|
|
||
| // First, unmarshal normally to get all fields | ||
| if err := json.Unmarshal(data, alias); err != nil { |
There was a problem hiding this comment.
wondering if this logic (UnmarshalJSON) can be shared for all redis clients?
|
|
||
| const ( | ||
| SyncFeatureFlagNotificaiton SycnFeatureFlag = "sync-feature-flag-notification" | ||
| SycnFeatureFlagDatafile SycnFeatureFlag = "sync-feature-flag-datafile" |
There was a problem hiding this comment.
Interesting - consistently misspelled all of them :)
| ## Use "redis" for fire-and-forget pub/sub (existing behavior) | ||
| ## Use "redis-streams" for persistent message delivery with retries and acknowledgment | ||
| default: "redis" | ||
| # default: "redis-streams" # Uncomment to enable Redis Streams |
There was a problem hiding this comment.
datafile sync is agent-internal. Do we need to care about this redis-streams option?
I see a value, but it may be confusing to the clients.
| } else if conf.Datafile.Default == PubSubRedisStreams { | ||
| return getPubSubRedisStreams(conf) |
There was a problem hiding this comment.
Same, this is agent-internal. Less value compared to the confusion? We can consider focusing on notification sync only for redis-stream support.
This commit integrates Redis Streams for persistent notification delivery from v4.3.0-beta.1 with the existing CMAB Redis cache support in master.
Features Added:
Changes:
Configuration:
Testing:
Both features coexist without conflicts - Redis Streams uses STREAM data type for messaging while CMAB cache uses STRING type for key-value storage.
Issues