Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

feat: [default polling to 10 seconds from 5 minutes; add configurable value] (FF-2686) #60

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

leoromanovsky
Copy link
Member

@leoromanovsky leoromanovsky commented Jul 10, 2024

… value] (FF-2686)


labels: mergeable

Fixes: #issue

Motivation and Context

The poller is hard-coded to refresh every 5 minutes; this is too long between updates.

Description

  • Default polling every 10 seconds with 2 second jitter.
  • Change API to use seconds as the unit instead of milliseconds - this seems more user friendly
  • Add ability to configure the polling interval in seconds.

How has this been tested?

There are some bandit data test failures not related to this change.

@leoromanovsky leoromanovsky marked this pull request as ready for review July 10, 2024 18:43
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Approving--despite the failing tests--as the change seem straightforward. Fine with 10 seconds, although most SDKs landed at the 30-second mark.

POLL_JITTER_MILLIS = 30 * SECOND_MILLIS
POLL_INTERVAL_MILLIS = 5 * MINUTE_MILLIS
POLL_JITTER_SECONDS = 2
POLL_INTERVAL_SECONDS = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to do 30 like our other SDKs?

Copy link
Member Author

Choose a reason for hiding this comment

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

golang is ten seconds - is 30 standard?

@leoromanovsky leoromanovsky merged commit d1b4fa7 into main Jul 15, 2024
3 of 4 checks passed
@leoromanovsky leoromanovsky deleted the lr/ff-2686 branch July 15, 2024 19:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants