-
Notifications
You must be signed in to change notification settings - Fork 1
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
Retry with backoff #30
base: main
Are you sure you want to change the base?
Conversation
1f22f51
to
2c35e7a
Compare
2c35e7a
to
149e9d3
Compare
retry-backoff/README.md
Outdated
initRetryWithBackoff, | ||
useRetryWithBackoff, | ||
} from "@effection-contrib/retry-backoff"; | ||
import { myOperation } from "./myOperation"; |
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.
Instead of doing the import, can we just inline the operation and use fetch so people see what that looks like?
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.
Done
retry-backoff/retry-backoff.ts
Outdated
export function* initRetryWithBackoff( | ||
defaults: RetryWithContextDefaults, | ||
) { | ||
// deno-lint-ignore require-yield | ||
function* init(): Operation<RetryWithContextDefaults> { | ||
return defaults; | ||
} | ||
|
||
return yield* ensureContext( | ||
RetryWithBackoffContext, | ||
init(), | ||
); | ||
} | ||
|
||
export function* ensureContext<T>(Context: ContextType<T>, init: Operation<T>) { | ||
if (!(yield* Context.get())) { | ||
yield* Context.set(yield* init); | ||
} | ||
return yield* Context.expect(); | ||
} |
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.
Since we're using default now, we can drop this. We don't need this code anymore
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.
Don't we still need it if someone wants to override the default for all retries?
retry-backoff/README.md
Outdated
import { myOperation } from "./myOperation"; | ||
|
||
await main(function* () { | ||
yield* initRetryWithBackoff({ timeout: 45_000 }); |
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.
Let's remove this function.
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.
Are you talking about the example myOperation
function? or main?
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.
the example myOperation function, so the example reads like
await main(function*() {
yield* retryBackoff(function*() {
const result = call(() => fetch(....));
...
});
})
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.
oh okay. I took care of that from this comment
retry-backoff/retry-backoff.ts
Outdated
timeout: number; | ||
} | ||
|
||
const RetryWithBackoffContext = createContext<RetryWithContextDefaults>( |
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.
Let's rename this to RetryBackoffContext
.
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.
Done
} | ||
); | ||
|
||
export function* useRetryWithBackoff<T> ( |
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 need docs for this function.
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.
Done
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.
What would be the right way to write docs for this context?
/**
* Description
*
* @property {function} set
* @param {{ timeout: number }}
*/
export const RetryBackoffContext = createContext<RetryWithContextDefaults>(
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.
That's right. You don't need to describe the parameters and properties because those will be generated from typescript
retry-backoff/README.md
Outdated
|
||
await main(function* () { | ||
yield* useRetryWithBackoff(function* () { | ||
yield* call(fetch("https://foo.bar/")); |
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.
This won't be compatible with v4, so let's use function syntax: yield* call(() => fetch(...))
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.
Done
retry-backoff/README.md
Outdated
await main(function* () { | ||
yield* initRetryWithBackoff({ timeout: 45_000 }); | ||
yield* retryWithBackoff(function* () { | ||
yield* call(fetch("https://foo.bar/")); |
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.
Same here, let's use the function syntax that's compatible with v4
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.
And done
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.
This is a very common use-case. What about just making this @effection-contrib/retry
and have many strategies (retriableWithBackoff) being just one.
interface RetriableOptions {
backoff(failedAttempts: number): number;
maxAttempts: number;
}
//. constant backoff
const retriable = createRetriable({ backoff: () => 3, maxAttempts: Math.Infinity});
yield* retriable(operation);
//exponential backoff
const retriable = createRetriable({ backoff: (attempts} => 2^attempts, maxAttempts: 100});
yield* retriable(operation);
You can even model the default as a the default that is on the context:
const defaultRetriable = createRetriable({ backoff: () => 10, maxAttempts: 10 });
const DefaulthRetriableContext = createContext<<T>(op: Operation<T>) => Operation<T>>('@effection-contrib/default-retriable', defaultRetriable);
export function* retriable<T>(operation: Operation<T>): Operation<T> {
let defaultImpl = yield* DefaultRetriableContext.expect();
return yield* defaultImpl(operation);
}
My suspicion is that most folks will want to roll their own.
Motivation
Sometimes when an API call fails, we want to be able to retry those requests until it succeeds (within a reasonable amount of time).
useRetryWithBackoff
will re-attempt an operation with incremental backoff until the configured timeout exceeds.Approach
There's a default timeout set to 30 seconds. If you'd like to set a different
timeout, you'll need to either pass in options as a second argument to
useRetryWithBackoff
:Or set the timeout via the context so that the same timeout can be applied to all
of your retry operations:
Learning
Screenshots