Skip to content

Commit

Permalink
revert preventing of retries for throttling errors (#20543)
Browse files Browse the repository at this point in the history 
This PR reverts the changes made here:
#19722

We decided that it would be better to have a more generalized solution
for the throttling errors, where hosts can disconnect the container when
they receive throttling error warnings.

Since there has been no new releases from FF with this change, the
revert seems safe. But would like reviewers to advise as well.
  • Loading branch information
@pragya91
pragya91 authored Apr 10, 2024
1 parent 0465748 commit 3410f15
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 129 deletions.
1 change: 0 additions & 1 deletion packages/drivers/odsp-driver-definitions/api-report/odsp-driver-definitions.api.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export interface HostStoragePolicy {
// (undocumented)
concurrentOpsBatches?: number;
concurrentSnapshotFetch?: boolean;
disableRetriesOnStorageThrottlingError?: boolean;
// @deprecated (undocumented)
enableRedeemFallback?: boolean;
// @deprecated (undocumented)
Expand Down
5 changes: 0 additions & 5 deletions packages/drivers/odsp-driver-definitions/src/factory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,4 @@ export interface HostStoragePolicy {
* as false. This is if the host wants to do some A/B testing.
*/
avoidPrefetchSnapshotCache?: boolean;

/**
* True if host does not want the storage service to perform retries when throttling errors occur in the service.
*/
disableRetriesOnStorageThrottlingError?: boolean;
}
4 changes: 1 addition & 3 deletions packages/drivers/odsp-driver/api-report/odsp-driver.api.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export function encodeOdspFluidDataStoreLocator(locator: OdspFluidDataStoreLocat

// @alpha
export class EpochTracker implements IPersistedFileCache {
constructor(cache: IPersistedCache, fileEntry: IFileEntry, logger: ITelemetryLoggerExt, clientIsSummarizer?: boolean | undefined, hostPolicy?: HostStoragePolicy | undefined);
constructor(cache: IPersistedCache, fileEntry: IFileEntry, logger: ITelemetryLoggerExt, clientIsSummarizer?: boolean | undefined);
// (undocumented)
protected readonly cache: IPersistedCache;
// (undocumented)
Expand All @@ -74,8 +74,6 @@ export class EpochTracker implements IPersistedFileCache {
// (undocumented)
get(entry: IEntry): Promise<any>;
// (undocumented)
protected readonly hostPolicy?: HostStoragePolicy | undefined;
// (undocumented)
protected readonly logger: ITelemetryLoggerExt;
// (undocumented)
put(entry: IEntry, value: any): Promise<void>;
Expand Down
60 changes: 18 additions & 42 deletions packages/drivers/odsp-driver/src/epochTracker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
ThrottlingError,
} from "@fluidframework/driver-utils/internal";
import {
type HostStoragePolicy,
ICacheEntry,
IEntry,
IFileEntry,
Expand Down Expand Up @@ -98,7 +97,6 @@ export class EpochTracker implements IPersistedFileCache {
protected readonly fileEntry: IFileEntry,
protected readonly logger: ITelemetryLoggerExt,
protected readonly clientIsSummarizer?: boolean,
protected readonly hostPolicy?: HostStoragePolicy,
) {
// Limits the max number of concurrent requests to 24.
this.rateLimiter = new RateLimiter(24);
Expand Down Expand Up @@ -297,48 +295,29 @@ export class EpochTracker implements IPersistedFileCache {
throw error;
})
.catch((error) => {
if (isFluidError(error)) {
// If the error is about location redirection, then we need to generate new resolved url with correct
// location info.
if (error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError) {
const redirectLocation = (error as IOdspErrorAugmentations)
.redirectLocation;
if (redirectLocation !== undefined) {
const redirectUrl: IOdspResolvedUrl = patchOdspResolvedUrl(
this.fileEntry.resolvedUrl,
redirectLocation,
);
const locationRedirectionError = new LocationRedirectionError(
error.message,
redirectUrl,
{ driverVersion, redirectLocation },
);
locationRedirectionError.addTelemetryProperties(
error.getTelemetryProperties(),
);
throw locationRedirectionError;
}
}
// If the hostPolicy disallows retries for throttling errors, then we throw a NonRetryableError
else if (
error.errorType === OdspErrorTypes.throttlingError &&
this.hostPolicy?.disableRetriesOnStorageThrottlingError
) {
const nonRetriableThrottlingError = new NonRetryableError(
// If the error is about location redirection, then we need to generate new resolved url with correct
// location info.
if (
isFluidError(error) &&
error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError
) {
const redirectLocation = (error as IOdspErrorAugmentations).redirectLocation;
if (redirectLocation !== undefined) {
const redirectUrl: IOdspResolvedUrl = patchOdspResolvedUrl(
this.fileEntry.resolvedUrl,
redirectLocation,
);
const locationRedirectionError = new LocationRedirectionError(
error.message,
OdspErrorTypes.throttlingError,
{
driverVersion,
},
redirectUrl,
{ driverVersion, redirectLocation },
);
// This step ensures all the telemetry props are preserved including the retryAfterSeconds from the original error
nonRetriableThrottlingError.addTelemetryProperties(
locationRedirectionError.addTelemetryProperties(
error.getTelemetryProperties(),
);
throw nonRetriableThrottlingError;
throw locationRedirectionError;
}
}

throw error;
})
.catch((error) => {
Expand Down Expand Up @@ -517,9 +496,8 @@ export class EpochTrackerWithRedemption extends EpochTracker {
protected readonly fileEntry: IFileEntry,
protected readonly logger: ITelemetryLoggerExt,
protected readonly clientIsSummarizer?: boolean,
protected readonly hostPolicy?: HostStoragePolicy,
) {
super(cache, fileEntry, logger, clientIsSummarizer, hostPolicy);
super(cache, fileEntry, logger, clientIsSummarizer);
// Handles the rejected promise within treesLatestDeferral.
this.treesLatestDeferral.promise.catch(() => {});
}
Expand Down Expand Up @@ -650,14 +628,12 @@ export function createOdspCacheAndTracker(
fileEntry: IFileEntry,
logger: ITelemetryLoggerExt,
clientIsSummarizer?: boolean,
hostPolicy?: HostStoragePolicy,
): ICacheAndTracker {
const epochTracker = new EpochTrackerWithRedemption(
persistedCacheArg,
fileEntry,
logger,
clientIsSummarizer,
hostPolicy,
);
return {
cache: {
Expand Down
2 changes: 0 additions & 2 deletions packages/drivers/odsp-driver/src/odspDocumentServiceFactoryCore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ export class OdspDocumentServiceFactoryCore
fileEntry,
odspLogger,
clientIsSummarizer,
this.hostPolicy,
);

return PerformanceEvent.timedExecAsync(
Expand Down Expand Up @@ -292,7 +291,6 @@ export class OdspDocumentServiceFactoryCore
{ resolvedUrl: odspResolvedUrl, docId: odspResolvedUrl.hashedDocumentId },
extLogger,
clientIsSummarizer,
this.hostPolicy,
);

const storageTokenFetcher = toInstrumentedOdspStorageTokenFetcher(
Expand Down
76 changes: 1 addition & 75 deletions packages/drivers/odsp-driver/src/test/epochTests.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import { strict as assert } from "node:assert";

import { IDocumentStorageServicePolicies } from "@fluidframework/driver-definitions/internal";
import { type NonRetryableError, ThrottlingError } from "@fluidframework/driver-utils/internal";
import {
ICacheEntry,
IEntry,
Expand All @@ -15,15 +14,13 @@ import {
maximumCacheDurationMs,
} from "@fluidframework/odsp-driver-definitions/internal";
import { type IFluidErrorBase, createChildLogger } from "@fluidframework/telemetry-utils/internal";
import { stub } from "sinon";

import { IVersionedValueWithEpoch, persistedCacheValueVersion } from "../contracts.js";
import { EpochTracker } from "../epochTracker.js";
import { LocalPersistentCache } from "../odspCache.js";
import { getHashedDocumentId } from "../odspPublicUtils.js";
import * as odspUtilsModule from "../odspUtils.js";

import { createResponse, mockFetchOk, mockFetchSingle } from "./mockFetch.js";
import { mockFetchOk, mockFetchSingle, createResponse } from "./mockFetch.js";

const createUtLocalCache = (): LocalPersistentCache => new LocalPersistentCache();

Expand Down Expand Up @@ -397,75 +394,4 @@ describe("Tests for Epoch Tracker", () => {
}
assert.strictEqual(success, false, "Fetching should not succeed!!");
});

it("Checks throttling errors are non-retriable when disableRetriesOnStorageThrottlingError=true", async () => {
const retryAfterSeconds = 1;
let fetchStub;
const epochTrackerWithHostPolicy = new EpochTracker(
localCache,
{
docId: hashedDocumentId,
resolvedUrl,
},
createChildLogger(),
undefined,
{ disableRetriesOnStorageThrottlingError: true } /* hostPolicy */,
);
try {
// fetchHelper is used by epochTracker's fetch method, which we stub here to emulate throttling error
fetchStub = stub(odspUtilsModule, "fetchHelper");
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
fetchStub.callsFake(async () => {
throw new ThrottlingError("Server is throttled", retryAfterSeconds, {
testProp: "testProp",
driverVersion: "1",
});
});
await epochTrackerWithHostPolicy.fetch("fetchUrl", {}, "test");
} catch (error) {
// retoring the fetchHelper function to avoid causing errors in other tests
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
fetchStub.restore();
assert(
(error as NonRetryableError<string>).canRetry === false,
"Error should be marked as non-retriable",
);
assert(
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
(error as any).retryAfterSeconds === retryAfterSeconds,
"retryAfterSeconds should exist",
);
}
});

it("Checks throttling errors retriable when disableRetriesOnStorageThrottlingError=false", async () => {
let fetchStub;
const epochTrackerWithHostPolicy = new EpochTracker(
localCache,
{
docId: hashedDocumentId,
resolvedUrl,
},
createChildLogger(),
undefined,
{ disableRetriesOnStorageThrottlingError: false } /* hostPolicy */,
);
try {
// fetchHelper is used by epochTracker's fetch method, which we stub here to emulate throttling error
fetchStub = stub(odspUtilsModule, "fetchHelper");
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
fetchStub.callsFake(async () => {
throw new ThrottlingError("Server is throttled", 1000, {
testProp: "testProp",
driverVersion: "1",
});
});
await epochTrackerWithHostPolicy.fetch("fetchUrl", {}, "test");
} catch (error) {
// retoring the fetchHelper function to avoid causing errors in other tests
// eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
fetchStub.restore();
assert((error as ThrottlingError).canRetry === true, "Error should be retriable");
}
});
});
1 change: 0 additions & 1 deletion packages/test/test-drivers/src/odspDriverApi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ export const generateOdspHostStoragePolicy = (seed: number) => {
enableShareLinkWithCreate: [false],
enableSingleRequestForShareLinkWithCreate: [false],
avoidPrefetchSnapshotCache: booleanCases,
disableRetriesOnStorageThrottlingError: booleanCases,
};
return generatePairwiseOptions<HostStoragePolicy>(odspHostPolicyMatrix, seed);
};

0 comments on commit 3410f15

Please sign in to comment.