- 
                Notifications
    You must be signed in to change notification settings 
- Fork 44
          Add retries to configureIndex and update operations
          #318
        
          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
Changes from 3 commits
1f1bfb8
              8df8e7d
              bb9a6de
              61b34d7
              3536e72
              6e357c1
              6c5ba34
              133b349
              28c1c64
              a04bdd0
              591b624
              60a8ad8
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1,15 +1,6 @@ | ||
| import { | ||
| BasePineconeError, | ||
| PineconeBadRequestError, | ||
| PineconeInternalServerError, | ||
| } from '../../errors'; | ||
| import { BasePineconeError, PineconeBadRequestError } from '../../errors'; | ||
| import { Pinecone } from '../../index'; | ||
| import { | ||
| randomIndexName, | ||
| retryDeletes, | ||
| sleep, | ||
| waitUntilReady, | ||
| } from '../test-helpers'; | ||
| import { randomIndexName, retryDeletes, waitUntilReady } from '../test-helpers'; | ||
|  | ||
| let podIndexName: string, serverlessIndexName: string, pinecone: Pinecone; | ||
|  | ||
|  | @@ -75,26 +66,10 @@ describe('configure index', () => { | |
| const description = await pinecone.describeIndex(podIndexName); | ||
| expect(description.spec.pod?.podType).toEqual('p1.x1'); | ||
|  | ||
| // Scale up podType to x2 | ||
| let state = true; | ||
| let retryCount = 0; | ||
| const maxRetries = 10; | ||
| while (state && retryCount < maxRetries) { | ||
| try { | ||
| await pinecone.configureIndex(podIndexName, { | ||
| spec: { pod: { podType: 'p1.x2' } }, | ||
| }); | ||
| state = false; | ||
| } catch (e) { | ||
| if (e instanceof PineconeInternalServerError) { | ||
| retryCount++; | ||
| await sleep(2000); | ||
| } else { | ||
| console.log('Unexpected error:', e); | ||
| throw e; | ||
| } | ||
| } | ||
| } | ||
| 
      Comment on lines
    
      -78
     to 
      -97
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need this now that we've got retries! | ||
| await pinecone.configureIndex(podIndexName, { | ||
| spec: { pod: { podType: 'p1.x2' } }, | ||
| }); | ||
|  | ||
| await waitUntilReady(podIndexName); | ||
| const description2 = await pinecone.describeIndex(podIndexName); | ||
| expect(description2.spec.pod?.podType).toEqual('p1.x2'); | ||
|  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -46,46 +46,46 @@ afterAll(async () => { | |
| }); | ||
|  | ||
| // todo: add sparse values update | ||
| describe('upsert and update to serverless index', () => { | ||
| test('verify upsert and update', async () => { | ||
| const recordToUpsert = generateRecords({ | ||
| dimension: 2, | ||
| quantity: 1, | ||
| withSparseValues: false, | ||
| withMetadata: true, | ||
| }); | ||
|  | ||
| // Upsert record | ||
| await serverlessIndex.upsert(recordToUpsert); | ||
|  | ||
| // Build new values | ||
| const newValues = [0.5, 0.4]; | ||
| const newMetadata = { flavor: 'chocolate' }; | ||
|  | ||
| const updateSpy = jest | ||
| .spyOn(serverlessIndex, 'update') | ||
| .mockResolvedValue(undefined); | ||
|  | ||
| // Update values w/new values | ||
| await serverlessIndex.update({ | ||
| id: '0', | ||
| values: newValues, | ||
| metadata: newMetadata, | ||
| }); | ||
|  | ||
| expect(updateSpy).toHaveBeenCalledWith({ | ||
| id: '0', | ||
| values: newValues, | ||
| metadata: newMetadata, | ||
| }); | ||
|  | ||
| // Clean up spy after the test | ||
| updateSpy.mockRestore(); | ||
| }); | ||
| }); | ||
| // describe('upsert and update to serverless index', () => { | ||
| // test('verify upsert and update', async () => { | ||
| // const recordToUpsert = generateRecords({ | ||
| // dimension: 2, | ||
| // quantity: 1, | ||
| // withSparseValues: false, | ||
| // withMetadata: true, | ||
| // }); | ||
| // | ||
| // // Upsert record | ||
| // await serverlessIndex.upsert(recordToUpsert); | ||
| // | ||
| // // Build new values | ||
| // const newValues = [0.5, 0.4]; | ||
| // const newMetadata = { flavor: 'chocolate' }; | ||
| // | ||
| // const updateSpy = jest | ||
| // .spyOn(serverlessIndex, 'update') | ||
| // .mockResolvedValue(undefined); | ||
| // | ||
| // // Update values w/new values | ||
| // await serverlessIndex.update({ | ||
| // id: '0', | ||
| // values: newValues, | ||
| // metadata: newMetadata, | ||
| // }); | ||
| // | ||
| // expect(updateSpy).toHaveBeenCalledWith({ | ||
| // id: '0', | ||
| // values: newValues, | ||
| // metadata: newMetadata, | ||
| // }); | ||
| // | ||
| // // Clean up spy after the test | ||
| // updateSpy.mockRestore(); | ||
| // }); | ||
| // }); | ||
|  | ||
| // Retry logic tests | ||
| describe('Testing retry logic on Upsert operation, as run on a mock, in-memory http server', () => { | ||
| describe('Testing retry logic via a mock, in-memory http server', () => { | ||
| const recordsToUpsert = generateRecords({ | ||
| dimension: 2, | ||
| quantity: 1, | ||
|  | @@ -96,13 +96,14 @@ describe('Testing retry logic on Upsert operation, as run on a mock, in-memory h | |
| let server: http.Server; // Note: server cannot be something like an express server due to conflicts w/edge runtime | ||
| let mockServerlessIndex: Index; | ||
| let callCount: number; | ||
| let op: string; | ||
|  | ||
| // Helper function to start the server with a specific response pattern | ||
| const startMockServer = (shouldSucceedOnSecondCall: boolean) => { | ||
| // Create http server | ||
| server = http.createServer((req, res) => { | ||
| const { pathname } = parse(req.url || '', true); | ||
| if (req.method === 'POST' && pathname === '/vectors/upsert') { | ||
| if (req.method === 'POST' && pathname === `/vectors/${op}`) { | ||
| callCount++; | ||
| if (shouldSucceedOnSecondCall && callCount === 1) { | ||
| res.writeHead(503, { 'Content-Type': 'application/json' }); | ||
|  | @@ -137,6 +138,7 @@ describe('Testing retry logic on Upsert operation, as run on a mock, in-memory h | |
| }); | ||
|  | ||
| test('Upsert operation should retry 1x if server responds 1x with error and 1x with success', async () => { | ||
| op = 'upsert'; | ||
| pinecone = new Pinecone({ | ||
| apiKey: process.env['PINECONE_API_KEY'] || '', | ||
| maxRetries: 2, | ||
|  | @@ -155,13 +157,47 @@ describe('Testing retry logic on Upsert operation, as run on a mock, in-memory h | |
| // Call Upsert operation | ||
| await mockServerlessIndex.upsert(recordsToUpsert); | ||
|  | ||
| // 2 total tries: 1 initial call, 1 retry | ||
| expect(retrySpy).toHaveBeenCalledTimes(1); // passes | ||
| expect(delaySpy).toHaveBeenCalledTimes(1); // fails | ||
| expect(callCount).toBe(2); | ||
| }); | ||
|  | ||
| test('Update operation should retry 1x if server responds 1x with error and 1x with success', async () => { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Figured duplicating this type of test across  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (We should rly centralize this type of thing to avoid duplicating this logic, but I think this is okay for now) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems useful to validate that the calls themselves trigger retries as we'd expect, is that what you're talking about centralizing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mmm I'm not 100% sure we're on the same page -- when you say "the calls themselves," are you talking about the different async funcs that we could pass into the  Assuming you answer yes to the above, yes that's what I'd like to centralize.... something like we have a single parameterized test that confirms that < whatever async func > is retried  | ||
| op = 'update'; | ||
|  | ||
| pinecone = new Pinecone({ | ||
| apiKey: process.env['PINECONE_API_KEY'] || '', | ||
| maxRetries: 2, | ||
| }); | ||
|  | ||
| mockServerlessIndex = pinecone | ||
| .Index(serverlessIndexName, 'http://localhost:4000') | ||
| .namespace(globalNamespaceOne); | ||
|  | ||
| const retrySpy = jest.spyOn(RetryOnServerFailure.prototype, 'execute'); | ||
| const delaySpy = jest.spyOn(RetryOnServerFailure.prototype, 'delay'); | ||
|  | ||
| // Start server with a successful response on the second call | ||
| startMockServer(true); | ||
|  | ||
| const recordIdToUpdate = recordsToUpsert[0].id; | ||
| const newMetadata = { flavor: 'chocolate' }; | ||
|  | ||
| // Call Update operation | ||
| await mockServerlessIndex.update({ | ||
| id: recordIdToUpdate, | ||
| metadata: newMetadata, | ||
| }); | ||
|  | ||
| // 2 total tries: 1 initial call, 1 retry | ||
| expect(retrySpy).toHaveBeenCalledTimes(1); | ||
| expect(delaySpy).toHaveBeenCalledTimes(1); | ||
| expect(callCount).toBe(2); | ||
| }); | ||
|  | ||
| test('Max retries exceeded w/o resolve', async () => { | ||
| op = 'upsert'; | ||
| pinecone = new Pinecone({ | ||
| apiKey: process.env['PINECONE_API_KEY'] || '', | ||
| maxRetries: 3, | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops forgot to map this the 1st time around