Skip to content
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

FFM-12087 New configuration options + close related bug fixes #203

Merged
merged 21 commits into from
Oct 4, 2024

Conversation

erdirowlands
Copy link
Contributor

@erdirowlands erdirowlands commented Oct 3, 2024

What

This PR adds two new features, and fixes some bugs around closing the SDK.

New Features

Retries

  • Adds new maxRequestRetry option, defaulting to 10, which controls the number of retries made for requests to auth / polling / metrics / fetch flag and fetch group requests

  • Changes the stream reconnect limit from 5 attempts to no limit. This was a bug, and to align with our SDKs the stream should always attempt to reconnect (on retryable errors)

  • Exponential backoff capped at 1 minute max delay.

Flush metrics when SDK is closed

  • Adds new BaseConfig option flushAnalyticsOnClose, defaulting to disabled, which will post analytics when the SDK is closed. The default timeout for this request is 30000ms, before which the SDK will not be fully closed and network resources used by the request will remain open.
  • Adds new HarnessConfig option flushAnalyticsOnCloseTimeout, defaulting to 30000ms, which can help if the user prefers a shorter or longer timeout before all SDK resources are closed.

Bug fixes

  • The stream executor service remained open when the sdk was closed, causing a memory leak. Now calls .executorService().shutdown();
  • Stops making a final poll request when the SDK is closed, which causes noisy error logs as network resources are evicted and the request fails.

Testing

  • Updated unit tests for request retries and metrics flushing
  • Extensive manual testing with sample application.

Issues

#201
#202

@erdirowlands erdirowlands changed the title FFM-12087 Adjust retry strategy FFM-12087 New configuration options + close related bug fixes Oct 3, 2024
Copy link
Contributor

@akiraqb akiraqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heck of a PR, i think it looks good my only concern would be, do we want to have a single retry value for all these scenarios.

@@ -48,7 +60,7 @@ public static void main(String[] args) {
TimeUnit.SECONDS);

// SDK will exit after 15 minutes, this gives the example time to stream events
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe worth updating the comment in line?

@erdirowlands
Copy link
Contributor Author

Heck of a PR, i think it looks good my only concern would be, do we want to have a single retry value for all these scenarios.

Thanks! Addressed your concern on slack but will put it here as well:

If streaming is enabled, then 99.9 percent of cases it should try to reconnect forever with exponential back off. I've only ever encountered one edge case in 2 years where a customer wants to limit stream retries for a client session, and that was on the browser (JS SDK) so we added a stream retry config there

FFM-12087 Bump version
@erdirowlands erdirowlands merged commit 866809f into main Oct 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants