Skip to content

Add demand control for uplink #1564

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
merged 10 commits into from
Mar 10, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gateway-js/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- _Nothing yet. Stay tuned!_
- Respect the `minDelaySeconds` returning from Uplink when polling and retrying to fetch the supergraph schema from Uplink [PR #1564](https://github.com/apollographql/federation/pull/1564)

## v2.0.0-preview.2

Expand Down
2 changes: 1 addition & 1 deletion gateway-js/src/__generated__/graphqlTypes.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions gateway-js/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export interface ServiceDefinitionUpdate {
export interface SupergraphSdlUpdate {
id: string;
supergraphSdl: string;
minDelaySeconds?: number;
}

export function isSupergraphSdlUpdate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ describe('loadSupergraphSdlFromStorage', () => {
compositionId: "originalId-1234",
maxRetries: 1,
roundRobinSeed: 0,
earliestFetchTime: null,
});

expect(result).toMatchObject({
Expand All @@ -88,6 +89,7 @@ describe('loadSupergraphSdlFromStorage', () => {
compositionId: "originalId-1234",
maxRetries: 1,
roundRobinSeed: 0,
earliestFetchTime: null,
}),
).rejects.toThrowError(
new UplinkFetcherError(
Expand Down Expand Up @@ -388,10 +390,49 @@ describe("loadSupergraphSdlFromUplinks", () => {
compositionId: "id-1234",
maxRetries: 5,
roundRobinSeed: 0,
earliestFetchTime: null,
});

expect(result).toBeNull();
expect(fetcher).toHaveBeenCalledTimes(1);
});

it("Waits the correct time before retrying", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't use mocked timers here because there's an issue with jest and nock that me and Jeffery Burt couldn't figure out how to solve. Using real timers here as a workaround and not writing a test for the polling/math.max with pollIntervalInMs for this reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const timeoutSpy = jest.spyOn(global, 'setTimeout');

mockSupergraphSdlRequest('originalId-1234', mockCloudConfigUrl1).reply(500);
mockSupergraphSdlRequestIfAfter('originalId-1234', mockCloudConfigUrl2).reply(
200,
JSON.stringify({
data: {
routerConfig: {
__typename: 'RouterConfigResult',
id: 'originalId-1234',
supergraphSdl: getTestingSupergraphSdl()
},
},
}),
);
const fetcher = getDefaultFetcher();

await loadSupergraphSdlFromUplinks({
graphRef,
apiKey,
endpoints: [mockCloudConfigUrl1, mockCloudConfigUrl2],
errorReportingEndpoint: undefined,
fetcher: fetcher,
compositionId: "originalId-1234",
maxRetries: 1,
roundRobinSeed: 0,
earliestFetchTime: new Date(Date.now() + 1000),
});

// test if setTimeout was called with a value in range to deal with time jitter
const setTimeoutCall = timeoutSpy.mock.calls[1][1];
expect(setTimeoutCall).toBeLessThanOrEqual(1000);
expect(setTimeoutCall).toBeGreaterThanOrEqual(900);

timeoutSpy.mockRestore();
});
});

22 changes: 18 additions & 4 deletions gateway-js/src/supergraphManagers/UplinkFetcher/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ export class UplinkFetcher implements SupergraphManager {
process.env.APOLLO_OUT_OF_BAND_REPORTER_ENDPOINT ?? undefined;
private compositionId?: string;
private fetchCount: number = 0;
private minDelayMs: number | null = null;
private earliestFetchTime: Date | null = null;

constructor(options: UplinkFetcherOptions) {
this.config = options;
Expand All @@ -46,7 +48,12 @@ export class UplinkFetcher implements SupergraphManager {

let initialSupergraphSdl: string | null = null;
try {
initialSupergraphSdl = await this.updateSupergraphSdl();
const result = await this.updateSupergraphSdl();
initialSupergraphSdl = result?.supergraphSdl || null;
if (result?.minDelaySeconds) {
this.minDelayMs = 1000 * result?.minDelaySeconds;
this.earliestFetchTime = new Date(Date.now() + this.minDelayMs);
}
} catch (e) {
this.logUpdateFailure(e);
throw e;
Expand Down Expand Up @@ -83,6 +90,7 @@ export class UplinkFetcher implements SupergraphManager {
compositionId: this.compositionId ?? null,
maxRetries: this.config.maxRetries,
roundRobinSeed: this.fetchCount++,
earliestFetchTime: this.earliestFetchTime,
});

if (!result) {
Expand All @@ -91,7 +99,8 @@ export class UplinkFetcher implements SupergraphManager {
this.compositionId = result.id;
// the healthCheck fn is only assigned if it's enabled in the config
await this.healthCheck?.(result.supergraphSdl);
return result.supergraphSdl;
const { supergraphSdl, minDelaySeconds } = result;
return { supergraphSdl, minDelaySeconds };
}
}

Expand All @@ -107,7 +116,12 @@ export class UplinkFetcher implements SupergraphManager {

this.state.pollingPromise = pollingPromise;
try {
const maybeNewSupergraphSdl = await this.updateSupergraphSdl();
const result = await this.updateSupergraphSdl();
const maybeNewSupergraphSdl = result?.supergraphSdl || null;
if (result?.minDelaySeconds) {
this.minDelayMs = 1000 * result?.minDelaySeconds;
this.earliestFetchTime = new Date(Date.now() + this.minDelayMs);
}
if (maybeNewSupergraphSdl) {
this.update?.(maybeNewSupergraphSdl);
}
Expand All @@ -118,7 +132,7 @@ export class UplinkFetcher implements SupergraphManager {
}

this.poll();
}, this.config.pollIntervalInMs);
}, this.minDelayMs ? Math.max(this.minDelayMs, this.config.pollIntervalInMs) : this.config.pollIntervalInMs);
}

private logUpdateFailure(e: any) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const SUPERGRAPH_SDL_QUERY = /* GraphQL */`#graphql
... on RouterConfigResult {
id
supergraphSdl: supergraphSDL
minDelaySeconds
}
... on FetchError {
code
Expand Down Expand Up @@ -56,6 +57,7 @@ export async function loadSupergraphSdlFromUplinks({
compositionId,
maxRetries,
roundRobinSeed,
earliestFetchTime,
}: {
graphRef: string;
apiKey: string;
Expand All @@ -65,6 +67,7 @@ export async function loadSupergraphSdlFromUplinks({
compositionId: string | null;
maxRetries: number,
roundRobinSeed: number,
earliestFetchTime: Date | null
}) : Promise<SupergraphSdlUpdate | null> {
// This Promise resolves with either an updated supergraph or null if no change.
// This Promise can reject in the case that none of the retries are successful,
Expand All @@ -81,6 +84,10 @@ export async function loadSupergraphSdlFromUplinks({
}),
{
retries: maxRetries,
onRetry: async () => {
const delayMS = earliestFetchTime ? earliestFetchTime.getTime() - Date.now(): 0;
if (delayMS > 0) await new Promise(resolve => setTimeout(resolve, delayMS));
}
},
);

Expand Down Expand Up @@ -176,9 +183,10 @@ export async function loadSupergraphSdlFromStorage({
const {
id,
supergraphSdl,
minDelaySeconds,
// messages,
} = routerConfig;
return { id, supergraphSdl: supergraphSdl! };
return { id, supergraphSdl: supergraphSdl!, minDelaySeconds };
} else if (routerConfig.__typename === 'FetchError') {
// FetchError case
const { code, message } = routerConfig;
Expand Down