Skip to content

Commit df20ed7

Browse files
Mat001Mat001
committed
Removed force_implementation config field and comments about manual override
1 parent d3125ea commit df20ed7

File tree

6 files changed

+82
-148
lines changed

6 files changed

+82
-148
lines changed

‎.github/workflows/agent.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ jobs:
5353
- name: Start Redis
5454
uses: supercharge/redis-github-action@1.5.0
5555
with:
56-
redis-version: 6
56+
redis-version: 5
5757
- name: coveralls
5858
id: coveralls
5959
run: |
@@ -115,7 +115,7 @@ jobs:
115115
- name: Start Redis
116116
uses: supercharge/redis-github-action@1.5.0
117117
with:
118-
redis-version: 6
118+
redis-version: 5
119119
- name: acceptance test
120120
run: |
121121
make -e setup build

‎config.yaml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -306,11 +306,6 @@ synchronization:
306306
# max_retry_delay: 5s # Max retry delay with backoff (default: 5s)
307307
# connection_timeout: 10s # Redis connection timeout (default: 10s)
308308

309-
## Manual override (optional): Force specific implementation, bypassing auto-detection
310-
## Only use if auto-detection fails or you need explicit control
311-
## Options: "streams" (requires Redis >= 5.0) or "pubsub" (works with any Redis)
312-
# force_implementation: "streams"
313-
314309
## if notification synchronization is enabled, then the active notification event-stream API
315310
## will get the notifications from available replicas
316311
notification:

‎docs/redis-streams.md‎

Lines changed: 1 addition & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -279,11 +279,6 @@ synchronization:
279279
max_retry_delay: 5s # Max retry delay (default: 5s)
280280
connection_timeout: 10s # Connection timeout (default: 10s)
281281

282-
# Optional: Force specific implementation (bypasses auto-detection)
283-
# Only use if auto-detection fails or you need explicit control
284-
# Options: "streams" (requires Redis >= 5.0) or "pubsub" (any Redis)
285-
# force_implementation: "streams"
286-
287282
notification:
288283
enable: true
289284
default: "redis" # Agent auto-detects best option based on Redis version
@@ -341,25 +336,7 @@ INFO Auto-detecting Redis version to choose best notification implementation...
341336
WARN Could not detect Redis version - will use Pub/Sub as safe fallback error="NOPERM"
342337
```
343338

344-
**Manual Override:**
345-
346-
If auto-detection fails (e.g., `INFO` command is restricted), you can force a specific implementation:
347-
348-
```yaml
349-
synchronization:
350-
pubsub:
351-
redis:
352-
force_implementation: "streams" # or "pubsub"
353-
```
354-
355-
- `"streams"` - Always use Redis Streams (fails if Redis < 5.0)
356-
- `"pubsub"` - Always use Redis Pub/Sub (works with any Redis version)
357-
358-
**When to use manual override:**
359-
- Auto-detection fails due to restricted Redis permissions
360-
- Testing specific implementation behavior
361-
- Debugging issues with auto-detection
362-
- Explicit control over implementation choice
339+
> **Note:** If auto-detection fails, Agent safely falls back to Redis Pub/Sub (compatible with all Redis versions).
363340

364341
### Configuration Parameters
365342

@@ -436,16 +413,6 @@ synchronization:
436413
- Redis 5+ will automatically use Streams
437414
- No breaking changes
438415

439-
**Scenario 3: Want to force Streams on Redis 5+**
440-
```yaml
441-
synchronization:
442-
pubsub:
443-
redis:
444-
force_implementation: "streams" # Explicit Streams
445-
notification:
446-
default: "redis"
447-
```
448-
449416
All Redis Streams configuration is backward compatible - existing `pubsub.redis` settings are reused.
450417

451418
## Testing
@@ -614,32 +581,6 @@ redis-cli PUBSUB CHANNELS "optimizely-sync-*"
614581
# They will expire naturally when no longer used
615582
```
616583

617-
---
618-
619-
### Manual Override (Force Specific Implementation)
620-
621-
If auto-detection fails or you need explicit control:
622-
623-
**Force Redis Streams (Redis >= 5.0):**
624-
625-
```yaml
626-
synchronization:
627-
pubsub:
628-
redis:
629-
force_implementation: "streams"
630-
```
631-
632-
**Force Redis Pub/Sub (any Redis version):**
633-
634-
```yaml
635-
synchronization:
636-
pubsub:
637-
redis:
638-
force_implementation: "pubsub"
639-
```
640-
641-
Restart Agent. No data migration needed.
642-
643584
## Troubleshooting
644585

645586
### Messages Not Delivered

‎pkg/syncer/pubsub.go‎

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -65,18 +65,7 @@ func newPubSub(conf config.SyncConfig, featureFlag SyncFeatureFlag) (PubSub, err
6565
if defaultPubSub == PubSubRedisStreams {
6666
return getPubSubRedisStreams(conf)
6767
} else if defaultPubSub == PubSubRedis {
68-
// Check if user wants to force a specific implementation
69-
forceImpl := getForceImplementation(conf)
70-
71-
if forceImpl == "streams" {
72-
log.Info().Msg("force_implementation=streams - using Redis Streams (bypassing auto-detection)")
73-
return getPubSubRedisStreams(conf)
74-
} else if forceImpl == "pubsub" {
75-
log.Info().Msg("force_implementation=pubsub - using Redis Pub/Sub (bypassing auto-detection)")
76-
return getPubSubRedis(conf)
77-
}
78-
79-
// No force - use auto-detection
68+
// Use auto-detection (with fallback to Pub/Sub if detection fails)
8069
return getPubSubWithAutoDetect(conf)
8170
}
8271

@@ -211,28 +200,6 @@ func getDurationFromConfig(config map[string]interface{}, key string, defaultVal
211200
return defaultValue
212201
}
213202

214-
// getForceImplementation extracts the force_implementation config value if present
215-
// Returns "streams", "pubsub", or empty string if not set
216-
func getForceImplementation(conf config.SyncConfig) string {
217-
pubsubConf, found := conf.Pubsub[PubSubRedis]
218-
if !found {
219-
return ""
220-
}
221-
222-
redisConf, ok := pubsubConf.(map[string]interface{})
223-
if !ok {
224-
return ""
225-
}
226-
227-
if val, found := redisConf["force_implementation"]; found {
228-
if strVal, ok := val.(string); ok {
229-
return strVal
230-
}
231-
}
232-
233-
return ""
234-
}
235-
236203
// getPubSubWithAutoDetect creates a PubSub instance using Redis version auto-detection
237204
// Falls back to Pub/Sub (safe default) if detection fails for any reason
238205
func getPubSubWithAutoDetect(conf config.SyncConfig) (PubSub, error) {

‎pkg/syncer/pubsub_test.go‎

Lines changed: 51 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,9 @@ func TestNewPubSub(t *testing.T) {
4343
conf: config.SyncConfig{
4444
Pubsub: map[string]interface{}{
4545
"redis": map[string]interface{}{
46-
"host": "localhost:6379",
47-
"password": "",
48-
"database": 0,
49-
"force_implementation": "pubsub", // Force Pub/Sub for deterministic test
46+
"host": "localhost:6379",
47+
"password": "",
48+
"database": 0,
5049
},
5150
},
5251
Notification: config.FeatureSyncConfig{
@@ -56,10 +55,16 @@ func TestNewPubSub(t *testing.T) {
5655
},
5756
flag: SyncFeatureFlagNotification,
5857
},
59-
want: &pubsub.Redis{
60-
Host: "localhost:6379",
61-
Password: "",
62-
Database: 0,
58+
want: &pubsub.RedisStreams{
59+
Host: "localhost:6379",
60+
Password: "",
61+
Database: 0,
62+
BatchSize: 10,
63+
FlushInterval: 5 * time.Second,
64+
MaxRetries: 3,
65+
RetryDelay: 100 * time.Millisecond,
66+
MaxRetryDelay: 5 * time.Second,
67+
ConnTimeout: 10 * time.Second,
6368
},
6469
wantErr: false,
6570
},
@@ -69,10 +74,9 @@ func TestNewPubSub(t *testing.T) {
6974
conf: config.SyncConfig{
7075
Pubsub: map[string]interface{}{
7176
"redis": map[string]interface{}{
72-
"host": "localhost:6379",
73-
"password": "",
74-
"database": 0,
75-
"force_implementation": "pubsub", // Force Pub/Sub for deterministic test
77+
"host": "localhost:6379",
78+
"password": "",
79+
"database": 0,
7680
},
7781
},
7882
Datafile: config.FeatureSyncConfig{
@@ -82,10 +86,16 @@ func TestNewPubSub(t *testing.T) {
8286
},
8387
flag: SyncFeatureFlagDatafile,
8488
},
85-
want: &pubsub.Redis{
86-
Host: "localhost:6379",
87-
Password: "",
88-
Database: 0,
89+
want: &pubsub.RedisStreams{
90+
Host: "localhost:6379",
91+
Password: "",
92+
Database: 0,
93+
BatchSize: 10,
94+
FlushInterval: 5 * time.Second,
95+
MaxRetries: 3,
96+
RetryDelay: 100 * time.Millisecond,
97+
MaxRetryDelay: 5 * time.Second,
98+
ConnTimeout: 10 * time.Second,
8999
},
90100
wantErr: false,
91101
},
@@ -187,9 +197,8 @@ func TestNewPubSub(t *testing.T) {
187197
conf: config.SyncConfig{
188198
Pubsub: map[string]interface{}{
189199
"redis": map[string]interface{}{
190-
"host": "localhost:6379",
191-
"database": 0,
192-
"force_implementation": "pubsub", // Force Pub/Sub for deterministic test
200+
"host": "localhost:6379",
201+
"database": 0,
193202
},
194203
},
195204
Notification: config.FeatureSyncConfig{
@@ -199,10 +208,16 @@ func TestNewPubSub(t *testing.T) {
199208
},
200209
flag: SyncFeatureFlagNotification,
201210
},
202-
want: &pubsub.Redis{
203-
Host: "localhost:6379",
204-
Password: "", // Empty password is valid (no auth required)
205-
Database: 0,
211+
want: &pubsub.RedisStreams{
212+
Host: "localhost:6379",
213+
Password: "", // Empty password is valid (no auth required)
214+
Database: 0,
215+
BatchSize: 10,
216+
FlushInterval: 5 * time.Second,
217+
MaxRetries: 3,
218+
RetryDelay: 100 * time.Millisecond,
219+
MaxRetryDelay: 5 * time.Second,
220+
ConnTimeout: 10 * time.Second,
206221
},
207222
wantErr: false,
208223
},
@@ -232,10 +247,9 @@ func TestNewPubSub(t *testing.T) {
232247
conf: config.SyncConfig{
233248
Pubsub: map[string]interface{}{
234249
"redis": map[string]interface{}{
235-
"host": "localhost:6379",
236-
"password": 1234, // Invalid type, will be ignored
237-
"database": 0,
238-
"force_implementation": "pubsub", // Force Pub/Sub for deterministic test
250+
"host": "localhost:6379",
251+
"password": 1234, // Invalid type, will be ignored
252+
"database": 0,
239253
},
240254
},
241255
Notification: config.FeatureSyncConfig{
@@ -245,10 +259,16 @@ func TestNewPubSub(t *testing.T) {
245259
},
246260
flag: SyncFeatureFlagNotification,
247261
},
248-
want: &pubsub.Redis{
249-
Host: "localhost:6379",
250-
Password: "", // Invalid type ignored, falls back to empty string
251-
Database: 0,
262+
want: &pubsub.RedisStreams{
263+
Host: "localhost:6379",
264+
Password: "", // Invalid type ignored, falls back to empty string
265+
Database: 0,
266+
BatchSize: 10,
267+
FlushInterval: 5 * time.Second,
268+
MaxRetries: 3,
269+
RetryDelay: 100 * time.Millisecond,
270+
MaxRetryDelay: 5 * time.Second,
271+
ConnTimeout: 10 * time.Second,
252272
},
253273
wantErr: false,
254274
},

‎pkg/syncer/syncer_test.go‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"context"
2222
"reflect"
2323
"testing"
24+
"time"
2425

2526
"github.com/optimizely/agent/config"
2627
"github.com/optimizely/agent/pkg/syncer/pubsub"
@@ -65,10 +66,9 @@ func TestNewSyncedNotificationCenter(t *testing.T) {
6566
conf: config.SyncConfig{
6667
Pubsub: map[string]interface{}{
6768
"redis": map[string]interface{}{
68-
"host": "localhost:6379",
69-
"password": "",
70-
"database": 0,
71-
"force_implementation": "pubsub", // Force Pub/Sub for deterministic test
69+
"host": "localhost:6379",
70+
"password": "",
71+
"database": 0,
7272
},
7373
},
7474
Notification: config.FeatureSyncConfig{
@@ -81,10 +81,16 @@ func TestNewSyncedNotificationCenter(t *testing.T) {
8181
ctx: context.Background(),
8282
logger: &log.Logger,
8383
sdkKey: "123",
84-
pubsub: &pubsub.Redis{
85-
Host: "localhost:6379",
86-
Password: "",
87-
Database: 0,
84+
pubsub: &pubsub.RedisStreams{
85+
Host: "localhost:6379",
86+
Password: "",
87+
Database: 0,
88+
BatchSize: 10,
89+
FlushInterval: 5 * time.Second,
90+
MaxRetries: 3,
91+
RetryDelay: 100 * time.Millisecond,
92+
MaxRetryDelay: 5 * time.Second,
93+
ConnTimeout: 10 * time.Second,
8894
},
8995
},
9096
wantErr: false,
@@ -150,10 +156,9 @@ func TestNewDatafileSyncer(t *testing.T) {
150156
conf: config.SyncConfig{
151157
Pubsub: map[string]interface{}{
152158
"redis": map[string]interface{}{
153-
"host": "localhost:6379",
154-
"password": "",
155-
"database": 0,
156-
"force_implementation": "pubsub", // Force Pub/Sub for deterministic test
159+
"host": "localhost:6379",
160+
"password": "",
161+
"database": 0,
157162
},
158163
},
159164
Datafile: config.FeatureSyncConfig{
@@ -163,10 +168,16 @@ func TestNewDatafileSyncer(t *testing.T) {
163168
},
164169
},
165170
want: &DatafileSyncer{
166-
pubsub: &pubsub.Redis{
167-
Host: "localhost:6379",
168-
Password: "",
169-
Database: 0,
171+
pubsub: &pubsub.RedisStreams{
172+
Host: "localhost:6379",
173+
Password: "",
174+
Database: 0,
175+
BatchSize: 10,
176+
FlushInterval: 5 * time.Second,
177+
MaxRetries: 3,
178+
RetryDelay: 100 * time.Millisecond,
179+
MaxRetryDelay: 5 * time.Second,
180+
ConnTimeout: 10 * time.Second,
170181
},
171182
},
172183
wantErr: false,

0 commit comments

Comments
 (0)