-
Notifications
You must be signed in to change notification settings - Fork 573
Add configuration for consumer handling of retry on container creation #26698
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
Changes from 2 commits
0a7d74e
ca01549
83dd4d5
f443bef
e9ebaeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { delay } from "@fluidframework/core-utils/internal"; | |
| import { DriverErrorTypes } from "@fluidframework/driver-definitions/internal"; | ||
| import { | ||
| isFluidError, | ||
| wrapError, | ||
| type ITelemetryLoggerExt, | ||
| } from "@fluidframework/telemetry-utils/internal"; | ||
|
|
||
|
|
@@ -45,6 +46,12 @@ export interface IProgress { | |
| * @param error - error object returned from the call. | ||
| */ | ||
| onRetry?(delayInMs: number, error: unknown): void; | ||
|
|
||
| /** | ||
| * Maximum number of retries before giving up on a retriable error. | ||
| * If undefined, retries will continue indefinitely (default behavior). | ||
| */ | ||
| maxRetries?: number; | ||
kian-thompson marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -122,6 +129,35 @@ export async function runWithRetry<T>( | |
| } | ||
|
|
||
| numRetries++; | ||
|
|
||
| // Check if max retries limit has been reached | ||
| if (progress.maxRetries !== undefined && numRetries > progress.maxRetries) { | ||
| logger.sendTelemetryEvent( | ||
|
Comment on lines
+128
to
+130
|
||
| { | ||
| eventName: `${fetchCallName}_maxRetriesExceeded`, | ||
| retry: numRetries - 1, | ||
| maxRetries: progress.maxRetries, | ||
| duration: performanceNow() - startTime, | ||
| fetchCallName, | ||
| }, | ||
| error, | ||
| ); | ||
| // Wrap the original error to preserve its details while marking it non-retriable | ||
| throw wrapError( | ||
| error, | ||
| (message) => | ||
| new NonRetryableError( | ||
| `runWithRetry failed after max retries: ${message}`, | ||
| DriverErrorTypes.genericError, | ||
| { | ||
| driverVersion: pkgVersion, | ||
| fetchCallName, | ||
| maxRetries: progress.maxRetries, | ||
| }, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| lastError = error; | ||
| // Wait for the calculated time before retrying. | ||
| retryAfterMs = calculateMaxWaitTime(retryAfterMs, error); | ||
|
|
||
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.
Is this flag name already locked in? To me its naming is unintuitive -- it sounds like it's telling the Fluid Container to do retries, but instead its saying the caller will do the retries.
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.
Also, does it make sense to give the caller control over how many retries to do? If they want to disable it, set it to zero, but that way it gives them the opportunity to optimize for their specific startup time requirements
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.
+1, MaxRetriesOnAttachFailure seems nice, otherwise DisableRetryOnAttachFailure maybe
Uh oh!
There was an error while loading. Please reload this page.
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.
@anthony-murphy Thoughts on changing the name to something like
Fluid.Container.DisableRetryOnAttachFailure?The important thing here is that the errors are bubbled up to the consumer of
attach. I don't have any strong preference on how consumers can implement a retry mechanism, but retries are not necessary for this ask.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 flag primarily allows attach to be retired by the consumer, as without it the container will close when attach fails
Uh oh!
There was an error while loading. Please reload this page.
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 goal is to get out of the container level retry game here, and let the consumer decide what they want to do, retry calling, serialize the container and save it somewhere, or dispose the container throw it away
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 changed the flag name to
DisableCloseOnAttachFailureto be extra clear about what it does