From 0bb72ef0fc3f5a65c06ba7fcbd83b4c621cb2aad Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Thu, 6 May 2021 20:28:22 +0000 Subject: [PATCH] fix(endpoint-cache): revert removal of additional logic in get operation TimeStream Service Developer Guide refers to CachePeriodInMinutes as Time to Live (TTL) value. It also specifies calls to be made for the duration of TTL. Refs: https://a.co/2e8J7xT --- packages/endpoint-cache/README.md | 3 +- .../endpoint-cache/src/EndpointCache.spec.ts | 89 +++++++++++++------ packages/endpoint-cache/src/EndpointCache.ts | 24 ++++- 3 files changed, 84 insertions(+), 32 deletions(-) diff --git a/packages/endpoint-cache/README.md b/packages/endpoint-cache/README.md index 6d7257407c6d..5d72b8c4e41e 100644 --- a/packages/endpoint-cache/README.md +++ b/packages/endpoint-cache/README.md @@ -13,4 +13,5 @@ You probably shouldn't, at least directly. - uses `mnemonist/lru-cache` for storing the cache. - the `set` operation stores milliseconds elapsed since the UNIX epoch in Expires param based on CachePeriodInMinutes provided in Endpoint. -- the `get` operation returns all endpoints with their Expires values. +- the `get` operation returns all un-expired endpoints with their Expires values. +- the `getEndpoint` operation returns a randomly selected un-expired endpoint. diff --git a/packages/endpoint-cache/src/EndpointCache.spec.ts b/packages/endpoint-cache/src/EndpointCache.spec.ts index 60c36cf03f78..884f21c2f3d1 100644 --- a/packages/endpoint-cache/src/EndpointCache.spec.ts +++ b/packages/endpoint-cache/src/EndpointCache.spec.ts @@ -28,6 +28,9 @@ describe(EndpointCache.name, () => { Expires: now + CachePeriodInMinutes * 60 * 1000, })); + const getMaxCachePeriodInMins = (endpoints: Endpoint[]) => + Math.max(...endpoints.map((endpoint) => endpoint.CachePeriodInMinutes)); + beforeEach(() => { ((LRUCache as unknown) as jest.Mock).mockReturnValueOnce({ set, @@ -57,41 +60,71 @@ describe(EndpointCache.name, () => { jest.spyOn(Date, "now").mockImplementation(() => now); }); - describe("returns undefined", () => { - it("if cache doesn't have key", () => { - has.mockReturnValueOnce(false); - expect(endpointCache.get(key)).toBeUndefined(); - expect(has).toHaveBeenCalledWith(key); - expect(peek).not.toHaveBeenCalled(); - expect(get).not.toHaveBeenCalled(); - expect(set).not.toHaveBeenCalled(); - }); + const verifyHasAndGetCalls = () => { + expect(has).toHaveBeenCalledTimes(1); + expect(has).toHaveBeenCalledWith(key); + expect(get).toHaveBeenCalledTimes(1); + expect(get).toHaveBeenCalledWith(key); + }; - it("if cache has empty array", () => { - peek.mockReturnValueOnce([]); - expect(endpointCache.get(key)).toBeUndefined(); - expect(has).toHaveBeenCalledWith(key); - expect(peek).toHaveBeenCalledWith(key); - expect(get).not.toHaveBeenCalled(); + it("returns undefined if cache doesn't have key", () => { + has.mockReturnValueOnce(false); + expect(endpointCache.get(key)).toBeUndefined(); + expect(has).toHaveBeenCalledTimes(1); + expect(has).toHaveBeenCalledWith(key); + expect(peek).not.toHaveBeenCalled(); + expect(get).not.toHaveBeenCalled(); + }); + + it("returns undefined if cache has empty array", () => { + has.mockReturnValueOnce(true); + peek.mockReturnValueOnce([]); + expect(endpointCache.get(key)).toBeUndefined(); + expect(has).toHaveBeenCalledTimes(1); + expect(has).toHaveBeenCalledWith(key); + expect(peek).toHaveBeenCalledTimes(1); + expect(peek).toHaveBeenCalledWith(key); + expect(get).not.toHaveBeenCalled(); + }); + + it("returns undefined if cache returns undefined for key", () => { + get.mockReturnValueOnce(undefined); + expect(endpointCache.get(key)).toBeUndefined(); + verifyHasAndGetCalls(); + expect(set).not.toHaveBeenCalled(); + }); + + it("returns undefined if endpoints have expired", () => { + const maxCachePeriod = getMaxCachePeriodInMins(mockEndpoints); + jest.spyOn(Date, "now").mockImplementation(() => now + (maxCachePeriod + 1) * 60 * 1000); + expect(endpointCache.get(key)).toBeUndefined(); + verifyHasAndGetCalls(); + expect(set).toHaveBeenCalledTimes(1); + expect(set).toHaveBeenCalledWith(key, []); + }); + + describe("getEndpoint", () => { + it("returns one of the un-expired endpoints", () => { + expect(mockEndpoints.map((endpoint) => endpoint.Address)).toContain(endpointCache.getEndpoint(key)); + verifyHasAndGetCalls(); expect(set).not.toHaveBeenCalled(); }); - it("if cache returns undefined for key", () => { - get.mockReturnValueOnce(undefined); - expect(endpointCache.get(key)).toBeUndefined(); - expect(has).toHaveBeenCalledWith(key); - expect(peek).toHaveBeenCalledWith(key); - expect(get).toHaveBeenCalledWith(key); + it("returns un-expired endpoint", () => { + jest.spyOn(Date, "now").mockImplementation(() => now + 90 * 1000); + expect(endpointCache.getEndpoint(key)).toEqual(mockEndpoints[1].Address); + verifyHasAndGetCalls(); expect(set).not.toHaveBeenCalled(); }); - }); - it("returns endpoints if available", () => { - expect(endpointCache.get(key)).toEqual(getEndpointsWithExpiry(mockEndpoints)); - expect(has).toHaveBeenCalledWith(key); - expect(peek).toHaveBeenCalledWith(key); - expect(get).toHaveBeenCalledWith(key); - expect(set).not.toHaveBeenCalled(); + [0, 1].forEach((index) => { + it(`returns un-expired endpoint at index ${index}`, () => { + jest.spyOn(Math, "floor").mockImplementation(() => index); + expect(mockEndpoints.map((endpoint) => endpoint.Address)).toContain(endpointCache.getEndpoint(key)); + verifyHasAndGetCalls(); + expect(set).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/packages/endpoint-cache/src/EndpointCache.ts b/packages/endpoint-cache/src/EndpointCache.ts index e62f2006a272..5f61ae0a9e24 100644 --- a/packages/endpoint-cache/src/EndpointCache.ts +++ b/packages/endpoint-cache/src/EndpointCache.ts @@ -14,7 +14,22 @@ export class EndpointCache { } /** - * Returns endpoints for the given key. + * Returns an un-expired endpoint for the given key. + * + * @param endpointsWithExpiry + * @returns + */ + getEndpoint(key: string) { + const endpointsWithExpiry = this.get(key); + if (!endpointsWithExpiry || endpointsWithExpiry.length === 0) { + return undefined; + } + const endpoints = endpointsWithExpiry.map((endpoint) => endpoint.Address); + return endpoints[Math.floor(Math.random() * endpoints.length)]; + } + + /** + * Returns un-expired endpoints for the given key. * * @param key * @returns @@ -24,12 +39,15 @@ export class EndpointCache { return; } - const endpointsWithExpiry = this.cache.get(key); - if (!endpointsWithExpiry) { + const value = this.cache.get(key); + if (!value) { return; } + const now = Date.now(); + const endpointsWithExpiry = value.filter((endpoint) => now < endpoint.Expires); if (endpointsWithExpiry.length === 0) { + this.delete(key); return undefined; }