-
Notifications
You must be signed in to change notification settings - Fork 259
Add demand control for uplink #1564
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
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Can we add a test that demonstrates gateway utilizing this value? Additionally, a new test that exercises the Math.max logic (largest value overrides) would be good too. I'm wondering if |
}); | ||
|
||
expect(result).toBeNull(); | ||
expect(fetcher).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it("Waits the correct time before retrying", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't use mocked timers here because there's an issue with jest and nock that me and Jeffery Burt couldn't figure out how to solve. Using real timers here as a workaround and not writing a test for the polling/math.max with pollIntervalInMs
for this reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… as new polling interval Co-authored-by: Chris Lenfest <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test addition 👍 The code changes LGTM.
I'm still concerned about pollIntervalInMs
.
In managed mode, is there ever a case where the user's configuration is relevant after this change? Is it just the failure case?
I would be inclined to have a sane default which the gateway uses if it doesn't get one from Uplink and remove this notion of polling configurability altogether. Either that, or create a more appropriately named option for what this actually does now, which is more like fallbackPollIntervalInMs
.
I think we should get some other opinions on this from @apollographql/atlas.
From a quick look in the code it seems |
I like the idea of setting it as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Co-authored-by: Yangzi Guo <[email protected]>
Implements https://github.com/mdg-private/planning/issues/727
This PR uses the new
minDelaySeconds
returned by uplink to set the polling interval instead of gateways polling every 10 seconds by default configurable on the gateway side. If nominDelaySeconds
is provided by uplink,pollIntervalInMs
will still be used.