Skip to content
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

Fix IndexHostSingleton holding onto one instance of ManageIndexesAPI #224

Merged
merged 3 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
50 changes: 49 additions & 1 deletion src/data/__tests__/indexHostSingleton.test.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import { IndexHostSingleton } from '../indexHostSingleton';

const mockDescribeIndex = jest.fn();
const mockIndexOperationsBuilder = jest.fn();

jest.mock('../../control', () => {
const realControl = jest.requireActual('../../control');

return {
...realControl,
describeIndex: () => mockDescribeIndex,
indexOperationsBuilder: (config) => mockIndexOperationsBuilder(config),
};
});

describe('IndexHostSingleton', () => {
afterEach(() => {
IndexHostSingleton._reset();
mockDescribeIndex.mockReset();
mockIndexOperationsBuilder.mockReset();
});

test('calls describeIndex to resolve host for a specific apiKey and indexName, prepends protocol to host', async () => {
Expand Down Expand Up @@ -140,4 +142,50 @@ describe('IndexHostSingleton', () => {
await IndexHostSingleton.getHostUrl(pineconeConfig, 'test-index');
expect(mockDescribeIndex).toHaveBeenCalledTimes(1);
});

test.only('calling getHostUrl with different apiKey configurations should instantiate new ManageIndexesApi classes', async () => {
const pineconeConfig1 = { apiKey: 'test-key-1' };
const pineconeConfig2 = { apiKey: 'test-key-2' };

mockDescribeIndex
.mockResolvedValueOnce({
name: 'index-1',
dimensions: 10,
metric: 'cosine',
host: 'test-host-1',
spec: { pod: { pods: 1, replicas: 1, shards: 1, podType: 'p1.x1' } },
status: { ready: true, state: 'Ready' },
})
.mockResolvedValueOnce({
name: 'index-2',
dimensions: 10,
metric: 'cosine',
host: 'test-host-2',
spec: { pod: { pods: 1, replicas: 1, shards: 1, podType: 'p1.x1' } },
status: { ready: true, state: 'Ready' },
});
mockIndexOperationsBuilder.mockReturnValue({ test: 'one', test2: 'two' });

await IndexHostSingleton.getHostUrl(pineconeConfig1, 'index-1');
await IndexHostSingleton.getHostUrl(pineconeConfig2, 'index-2');

expect(mockDescribeIndex).toHaveBeenCalledTimes(2);
expect(mockDescribeIndex).toHaveBeenNthCalledWith(1, 'index-1');
expect(mockDescribeIndex).toHaveBeenNthCalledWith(2, 'index-2');

console.log(
'mockIndexOperationsBuilder',
JSON.stringify(mockIndexOperationsBuilder.mock)
);

expect(mockIndexOperationsBuilder).toHaveBeenCalledTimes(2);
expect(mockIndexOperationsBuilder).toHaveBeenNthCalledWith(
1,
pineconeConfig1
);
expect(mockIndexOperationsBuilder).toHaveBeenNthCalledWith(
2,
pineconeConfig2
);
});
});
27 changes: 11 additions & 16 deletions src/data/indexHostSingleton.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { ManageIndexesApi } from '../pinecone-generated-ts-fetch';
import type { PineconeConfiguration } from './types';
import type { IndexName } from '../control';
import { describeIndex, indexOperationsBuilder } from '../control';
Expand All @@ -10,16 +9,12 @@ import { normalizeUrl } from '../utils';
// and index, so we cache them in a singleton for reuse.
export const IndexHostSingleton = (function () {
const hostUrls = {}; // map of apiKey-indexName to hostUrl
let indexOperationsApi: InstanceType<typeof ManageIndexesApi> | null = null;

const _describeIndex = async (
config: PineconeConfiguration,
indexName: IndexName
): Promise<string> => {
if (!indexOperationsApi) {
indexOperationsApi = indexOperationsBuilder(config);
}

const indexOperationsApi = indexOperationsBuilder(config);
const describeResponse = await describeIndex(indexOperationsApi)(indexName);
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 really don't know what I was doing here, seems really silly to me now lol.

const host = describeResponse.host;

Expand All @@ -34,19 +29,17 @@ export const IndexHostSingleton = (function () {
}
};

const key = (config, indexName) => `${config.apiKey}-${indexName}`;
const _key = (config: PineconeConfiguration, indexName: string) =>
`${config.apiKey}-${indexName}`;
Copy link
Contributor Author

@austin-denoble austin-denoble May 31, 2024

Choose a reason for hiding this comment

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

Minor tweak, underscore just to make it more clear it's internal to the singleton.


return {
getHostUrl: async function (
config: PineconeConfiguration,
indexName: IndexName
) {
const cacheKey = key(config, indexName);
const singleton = {
getHostUrl: async (config: PineconeConfiguration, indexName: IndexName) => {
const cacheKey = _key(config, indexName);
if (cacheKey in hostUrls) {
return hostUrls[cacheKey];
} else {
const hostUrl = await _describeIndex(config, indexName);
this._set(config, indexName, hostUrl);
singleton._set(config, indexName, hostUrl);

Comment on lines +35 to +42
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is primarily a stylistic change, I wanted the context of this to be more clear. I think this reads a bit better overall but if anyone has any other feedback let me know.

Choose a reason for hiding this comment

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

How does this call to singleton._set work, since singleton doesn't exist yet?

Copy link

@ssmith-pc ssmith-pc May 31, 2024

Choose a reason for hiding this comment

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

Ok, I see now after talking this over with you. By the time the function that refers to singleton runs, singleton has already been initialized. Fancy!

if (!hostUrls[cacheKey]) {
throw new PineconeUnableToResolveHostError(
Expand Down Expand Up @@ -74,13 +67,15 @@ export const IndexHostSingleton = (function () {
return;
}

const cacheKey = key(config, indexName);
const cacheKey = _key(config, indexName);
hostUrls[cacheKey] = normalizedHostUrl;
},

_delete: (config: PineconeConfiguration, indexName: IndexName) => {
const cacheKey = key(config, indexName);
const cacheKey = _key(config, indexName);
delete hostUrls[cacheKey];
},
};

return singleton;
})();
Loading