Skip to content
This repository was 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
leoromanovsky merged 4 commits into
mainfrom
lr/ff-2686
Jul 15, 2024
Merged

feat: [default polling to 10 seconds from 5 minutes; add configurable value] (FF-2686)#60
leoromanovsky merged 4 commits into
mainfrom
lr/ff-2686

Conversation

@leoromanovsky

@leoromanovsky leoromanovsky commented Jul 10, 2024

Copy link
Copy Markdown
Member

… 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

@aarsilv aarsilv left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread eppo_client/constants.py Outdated
POLL_JITTER_MILLIS = 30 * SECOND_MILLIS
POLL_INTERVAL_MILLIS = 5 * MINUTE_MILLIS
POLL_JITTER_SECONDS = 2
POLL_INTERVAL_SECONDS = 10

Copy link
Copy Markdown
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
Copy Markdown
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
@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