Conversation
pvcraven
left a comment
There was a problem hiding this comment.
Looks ok, but some items claude found you might want to look at.
Issues & Concerns
Off-by-one Error in dispatcher.go (Critical)
In pkg/event/dispatcher.go:177, the retry interval calculation uses retryCount - 1:
time.Sleep(getRetryInterval(retryCount - 1))
This is called AFTER incrementing retryCount, which means:
First retry: retryCount=1, sleeps getRetryInterval(0) = 200ms ✓
Second retry: retryCount=2, sleeps getRetryInterval(1) = 400ms ✓
This appears correct, but it's confusing. The same pattern is used in both error paths (lines 177 and 186).
Inconsistent Retry Timing in ODP Event Manager
In pkg/odp/event/event_manager.go:311, the delay calculation uses retryCount-1 but applies the sleep BEFORE the next retry attempt:
retryCount++
if retryCount < maxRetries {
delay := initialRetryInterval * time.Duration(1<<(retryCount-1))
After first failure: retryCount=1, delay = 1<<0 = 200ms ✓
After second failure: retryCount=2, delay = 1<<1 = 400ms ✓
This is correct but uses inline calculation instead of the getRetryInterval helper function from dispatcher.go.
Missing Helper Function in ODP Code
The ODP event manager duplicates the exponential backoff logic inline instead of using a shared helper function like getRetryInterval from dispatcher.go. This creates maintenance burden and potential for inconsistency.
Test Timing Sensitivity
In pkg/utils/requester_test.go:280, the test expects:
assert.True(t, elapsed >= 2*time.Second && elapsed <= 10*time.Second, "took %s", elapsed)
The upper bound of 10 seconds is extremely generous. With 5 attempts and delays of 200ms + 400ms + 800ms + 1000ms = 2400ms, a more reasonable upper bound would be 3-4 seconds.
Changed Requester Retry Condition
In pkg/utils/requester.go:204, the condition changed from:
if i != r.retries {
to:
if i < r.retries-1 {
This needs verification: with retries=5 and loop i from 0 to 4:
Old: sleeps when i != 5 (all iterations)
New: sleeps when i < 4 (iterations 0-3, not 4)
This means no sleep after the last attempt, which is correct behavior but represents a subtle logic change that should be explicitly noted.
📝 Suggestions
Consolidate Helper Functions: Extract getRetryInterval to a shared utility package to avoid duplication between dispatcher.go and inline calculations elsewhere.
Add Code Comments: The 1<<retryCount bit-shift syntax for powers of 2 could use a comment explaining it calculates 2^retryCount for readers unfamiliar with this pattern.
Tighten Test Bounds: Reduce the overly generous 10-second upper bound in requester tests to something more realistic (3-4 seconds).
Consider Making Constants Configurable: While the requirements state "We are not going to provide configurable option," consider whether these should be exported constants that advanced users could override if needed in future.
User Story
First Step:
Validate whether this injection is supported in this language SDK
If Yes- Close the ticket as ‘done’ and a comment that this is already complete
If No- Move into the work needed. Add simple retry.
Set up so we can support the retry.
As a developer, I want to retry sending events that are not successfully sent to Optimizely, so that events are not dropped.
Acceptance Criteria
Jira ticket: https://optimizely-ext.atlassian.net/browse/FSSDK-12152