Repository navigation
feat: Retry event delivery and close the client with a timeout - #253
Conversation
Its entry in Flagsmith/sdk-spec's compatibility matrix at v0.9.0: `core events local-evaluation`.
Spec v0.7.1 runs the `@events` scenarios for this SDK: getting an experiment flag, tracking an event and tracking an exposure event, all with events enabled. The SDK already implements these (#226), so the steps file wires each step to FlagsmithClient.getExperimentFlag, trackEvent and trackExposureEvent, and creates the events-enabled client through FlagsmithConfig, next to the local evaluation client. (cherry picked from commit 0c48af8) Co-Authored-By: Claude Opus 5.5 <[email protected]> (cherry picked from commit 29cbe7b) (cherry picked from commit 723a712)
Spec v0.8.0 specifies how SDKs deliver events. The event processor now: - Times out each events request after 3 s, and gives a batch 3 attempts, backing off from 1 s, doubling, with full jitter. Network errors, timeouts, 408, 429, 500, 502, 503 and 504 are retried. - Keeps a batch whose last attempt failed transiently in the buffer, ahead of newer events, for the next flush, unless that attempt got a 500. Any other error response drops the batch. - Stops event tracking, dropping the buffer, on a 401 or 403. - Sends one batch at a time, and caps the buffer at 1000 events by dropping the oldest, kept events included. - Never fails `flushEvents()`, which waits for the send, retries included. `FlagsmithClient.close(timeout, unit)` sends the buffered events for at most the timeout, then abandons the request in flight. `close()` waits for one batch's worst case, 12 s. The spec steps file wires the new steps: flushing events, closing with a timeout, tracking a range of events, and a client with both local evaluation and events enabled. Co-Authored-By: Claude Opus 5.5 <[email protected]> (cherry picked from commit 88fa258)
flagsmithsmith
Bot
added
the
sdk-spec
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Requesting changes. Spec passes and the structure is good, but events behind a kept batch are never sent on flush or close, and a full buffer after a kept batch retries on every track. Please also retitle: the tracking methods are already on main.
…hile A flush requested while a batch was in flight completed without sending the events tracked since, when that batch was kept, and close() then dropped them without trying. Such a flush now sends the whole buffer, the kept batch first, and so does close(). After a kept batch, a full buffer no longer starts a new retry cycle on every tracked event: it waits for the next timer tick or explicit flush. A batch that gets through lifts the hold. Co-Authored-By: Claude Opus 5.5 <[email protected]>
flagsmithsmith
Bot
changed the title
getExperimentFlag, trackEvent and trackExposure methods
|
Thanks @Zaimwa9. Addressed in f3c57d7.
Fixed. A flush waiting behind a kept batch now sends the whole buffer, kept batch first.
Fixed. After a kept batch, a full buffer waits for the next timer tick or explicit flush. The next batch that gets through lifts the hold. Details in the inline reply.
Retitled to "feat: Retry event delivery and close the client with a timeout". Locally, |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Requesting changes. Both earlier points are fixed, but with the flush timer disabled (withEventsFlushIntervalMillis(0)) one kept batch stops size-triggered delivery for the rest of the process.
With `withEventsFlushIntervalMillis(0)` no tick ever lifts the hold a kept batch puts on size-triggered flushes, so one kept batch stopped them for the rest of the process. Without a timer, the hold now expires 10 s (the default flush interval) after the batch was kept; the next full buffer then sends it. With a timer, the next tick still lifts it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks @Zaimwa9. Addressed in abdd2fd.
Fixed. With the timer off, the hold now expires 10 s after the batch was kept (the default flush interval), and the next full buffer sends the kept batch and the newer events. With a timer, the next tick still lifts the hold. There are two new tests; details are in the inline reply. Locally, |
Contributes to Flagsmith/sdk-spec#13
Contributes to Flagsmith/sdk-spec#23