-
Notifications
You must be signed in to change notification settings - Fork 4.6k
🪟 🐛 Poll backend for Free Connector Program enrollment success #22289
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
Merged
ambirdsall
merged 4 commits into
master
from
alex/poll-backend-for-FCP-enrollment-success
Feb 8, 2023
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
563a521
Add pollUntil utility for polling Promises
ambirdsall 3625674
poll backend for confirmed enrollment before showing success toast
ambirdsall 3a31dce
Put interval and maxTimeout inside options arg
ambirdsall db88c72
Give units w/ polling options: intervalMs and maxTimeoutMs
ambirdsall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import { pollUntil } from "./pollUntil"; | ||
|
||
// a toy promise that can be polled for a specific response | ||
const fourZerosAndThenSeven = () => { | ||
let _callCount = 0; | ||
return () => Promise.resolve([0, 0, 0, 0, 7][_callCount++]); | ||
}; | ||
// eslint-disable-next-line | ||
const truthyResponse = (x: any) => !!x; | ||
|
||
describe("pollUntil", () => { | ||
describe("when maxTimeoutMs is not provided", () => { | ||
it("calls the provided apiFn until condition returns true and resolves to its final return value", () => { | ||
const pollableFn = fourZerosAndThenSeven(); | ||
|
||
return expect(pollUntil(pollableFn, truthyResponse, { intervalMs: 1 })).resolves.toBe(7); | ||
}); | ||
}); | ||
|
||
describe("when condition returns true before maxTimeoutMs is reached", () => { | ||
it("calls the provided apiFn until condition returns true and resolves to its final return value", () => { | ||
const pollableFn = fourZerosAndThenSeven(); | ||
|
||
return expect(pollUntil(pollableFn, truthyResponse, { intervalMs: 1, maxTimeoutMs: 100 })).resolves.toBe(7); | ||
}); | ||
}); | ||
|
||
describe("when maxTimeoutMs is reached before condition returns true", () => { | ||
it("resolves to false", () => { | ||
const pollableFn = fourZerosAndThenSeven(); | ||
|
||
return expect(pollUntil(pollableFn, truthyResponse, { intervalMs: 100, maxTimeoutMs: 1 })).resolves.toBe(false); | ||
}); | ||
|
||
// Because the timing of the polling depends on both the provided `intervalMs` and the | ||
// execution time of `apiFn`, the timing of polling iterations isn't entirely | ||
// deterministic; it's precise enough for its job, but it's difficult to make precise | ||
// test assertions about polling behavior without long intervalMs/maxTimeoutMs bogging | ||
// down the test suite. | ||
it("calls its apiFn arg no more than (maxTimeoutMs / intervalMs) times", async () => { | ||
let _callCount = 0; | ||
let lastCalledValue = 999; | ||
const pollableFn = () => | ||
Promise.resolve([1, 2, 3, 4, 5][_callCount++]).then((val) => { | ||
lastCalledValue = val; | ||
return val; | ||
}); | ||
|
||
await pollUntil(pollableFn, (_) => false, { intervalMs: 20, maxTimeoutMs: 78 }); | ||
|
||
// In theory, this is what just happened: | ||
// | time elapsed | value (source) | | ||
// |--------------+-----------------| | ||
// | 0ms | 1 (poll) | | ||
// | 20ms | 2 (poll) | | ||
// | 40ms | 3 (poll) | | ||
// | 60ms | 4 (poll) | | ||
// | 78ms | false (timeout) | | ||
// | ||
// In practice, since the polling intervalMs isn't started until after `apiFn` | ||
// resolves to a value, the actual call counts are slightly nondeterministic. We | ||
// could ignore that fact with a slow enough intervalMs, but who wants slow tests? | ||
expect(lastCalledValue > 2).toBe(true); | ||
expect(lastCalledValue <= 4).toBe(true); | ||
}); | ||
}); | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { timer, delay, from, concatMap, takeWhile, last, raceWith, lastValueFrom, NEVER } from "rxjs"; | ||
|
||
// Known issues: | ||
// - the case where `apiFn` returns `false` and `condition(false) === true` is impossible to distinguish from a timeout | ||
export function pollUntil<ResponseType>( | ||
apiFn: () => Promise<ResponseType>, | ||
condition: (res: ResponseType) => boolean, | ||
options: { intervalMs: number; maxTimeoutMs?: number } | ||
) { | ||
const { intervalMs, maxTimeoutMs } = options; | ||
const poll$ = timer(0, intervalMs).pipe( | ||
concatMap(() => from(apiFn())), | ||
takeWhile((result) => !condition(result), true), | ||
last() | ||
); | ||
|
||
const timeout$ = maxTimeoutMs ? from([false]).pipe(delay(maxTimeoutMs)) : NEVER; | ||
|
||
return lastValueFrom(poll$.pipe(raceWith(timeout$))); | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We have pretty similar code in
airbyte/airbyte-webapp/src/packages/cloud/views/credits/CreditsPage/components/RemainingCredits.tsx
Lines 73 to 91 in 6a74106
pollUntil
utility.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.
Great callout. We also do a bunch of manual interval wrangling for the health check endpoint, too; it would be good to consolidate our polling approaches.
That said, it would be nice to get this change merged to close an annoying edge case first, so I'm inclined to ship those updates as a follow up PR.