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 all 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);
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 describeResponse = await describeIndex(indexOperationsApi)(indexName);
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) =>
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.

`${config.apiKey}-${indexName}`;

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;
})();
12 changes: 6 additions & 6 deletions src/integration/data/delete.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe('delete', () => {
}
};

assertWithRetries(() => ns.fetch(['0']), fetchAssertions);
await assertWithRetries(() => ns.fetch(['0']), fetchAssertions);

// Try deleting the record
await ns.deleteOne('0');
Expand All @@ -79,7 +79,7 @@ describe('delete', () => {
expect(stats.namespaces[namespace]).toBeUndefined();
};

assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
await assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
});

test('verify deleteMany with ids', async () => {
Expand Down Expand Up @@ -113,7 +113,7 @@ describe('delete', () => {
}
};

assertWithRetries(() => ns.fetch(['0']), fetchAssertions);
await assertWithRetries(() => ns.fetch(['0']), fetchAssertions);

// Try deleting 2 of 3 records
await ns.deleteMany(['0', '2']);
Expand All @@ -129,7 +129,7 @@ describe('delete', () => {
}
};

assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);
await assertWithRetries(() => ns.describeIndexStats(), deleteAssertions);

// Check that record id='1' still exists
const fetchAssertions2 = (results) => {
Expand All @@ -143,7 +143,7 @@ describe('delete', () => {
}
};

assertWithRetries(() => ns.fetch(['1']), fetchAssertions2);
await assertWithRetries(() => ns.fetch(['1']), fetchAssertions2);

// deleting non-existent records should not throw
await ns.deleteMany(['0', '1', '2', '3']);
Expand All @@ -159,6 +159,6 @@ describe('delete', () => {
}
};

assertWithRetries(() => ns.describeIndexStats(), deleteAssertions2);
await assertWithRetries(() => ns.describeIndexStats(), deleteAssertions2);
});
});
4 changes: 2 additions & 2 deletions src/integration/errorHandling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ describe('Error handling', () => {
await p.index('foo-index').query({ topK: 10, id: '1' });
} catch (e) {
const err = e as PineconeConnectionError;
expect(err.name).toEqual('PineconeAuthorizationError');
expect(err.name).toEqual('PineconeConnectionError');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We get the correct error here now, which is great.

expect(err.message).toEqual(
'The API key you provided was rejected while calling https://api.pinecone.io/indexes/foo-index. Please check your configuration values and try again. You can find the configuration values for your project in the Pinecone developer console at https://app.pinecone.io'
'Request failed to reach Pinecone. This can occur for reasons such as network problems that prevent the request from being completed, or a Pinecone API outage. Check your network connection, and visit https://status.pinecone.io/ to see whether any outages are ongoing.'
);
}
});
Expand Down
Loading