feat: Race top n nodes when making requests #25
Merged
Conversation
This file contains 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
Co-authored-by: Diego Rodríguez Baquero <[email protected]>
* Abort on error (#19) * feat: use controller from options if exists. abort fetch if error occurs. * test: check if external abort controller is used * build: move build output to dist/ folder * fix: newline * 0.1.1 * Build exports (#20) * chore: rename file * feat: add new entrypoint with exports. Switch Saturn to named export * build: expose entire module instead of just the default export * docs: update README * 0.2.0 * feat: include worker scopes when checking for browser runtime (#21) * 0.3.0 --------- Co-authored-by: Eric Guan <[email protected]>
guanzo
reviewed
Oct 24, 2023
src/client.js
Outdated
| const controllers = [] | ||
|
|
||
| const createFetchPromise = async (origin) => { | ||
| options.url = origin |
There was a problem hiding this comment.
Better for each function call to make a copy of the options object instead of sharing it and mutating fields.
Suggested change
| options.url = origin | |
| const fetchOptions = { ...options, url: origin } // need to rename the rest of the references |
This is only a shallow copy but should suffice.
guanzo
reviewed
Oct 24, 2023
guanzo
reviewed
Oct 24, 2023
guanzo
reviewed
Oct 24, 2023
AmeanAsad
changed the title
guanzo
reviewed
Oct 24, 2023
| if (fallbackCount > this.opts.fallbackLimit) { | ||
| return | ||
| } | ||
| opts.url = origin.url | ||
| if (opts.raceNodes) { | ||
| const origins = nodes.slice(i, i + Saturn.defaultRaceCount).map((node) => node.url) |
There was a problem hiding this comment.
So if we have nodes a,b,c,d,e, then the race groups are:
- a,b,c
- 1st fallback: b,c,d
- 2nd fallback: c,d,e
Is that right?
There was a problem hiding this comment.
That is correct. Will also note that this is not necessarily the best way to do it and has some obvious room for improvement. For example:
- If node c fails, it we might hit it twice again before another good fallback is contacted.
I think as a next step, the best way to do this is to remove nodes that fail and such that:
- initial group is a, b, c. Assume node c fails
- 1st fallback is a, b, d and so on
I think this can be better achieved with a hashring implementation but we can re-evaluate.
guanzo
approved these changes
Oct 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description