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

Expose an option in createIndex to suppress the 409 conflict error #125

Merged
merged 7 commits into from
Sep 23, 2023

Conversation

glody007
Copy link
Contributor

@glody007 glody007 commented Sep 21, 2023

Problem

To create an index if it does not exist requires two API calls:

  • Calls to get the list of indexes
  • Calls to create the index if missing from the list

Solution

Instead of making two separate API calls, the idea is to streamline the process into a single call by introducing an option within the createIndex function.

Specifically, this solution proposes the addition of an option called suppressConflicts that allows the function to suppress the "409 Conflict" error. By using this option, the createIndex function would attempt to create the index, but if it encounters a "409 Conflict" error (indicating that the index is already present), it would not raise an exception This would reduce the need for additional calls or error handling in the client code.

Type of change

  • New feature (non-breaking change which adds functionality)

Test Plan

Adding a test case to check if a specific error for conflict is raised when the suppressConflicts option is not true

@jhamon
Copy link
Collaborator

jhamon commented Sep 21, 2023

Thanks for the quick follow up to #117

This code looks good to me. The CI errors are just because PRs from forks can't access secrets (e.g. our Pinecone API credentials) needed to run integration tests so you don't need to worry about those.

The only other thing I'd like to see added to this is a unit test covering this 409 error case added in here. And perhaps also a little more content in the PR description since we use those as commit messages.

@glody007
Copy link
Contributor Author

Thank you for your feedback. I've already added a unit test to cover the 409 error case, and I've provided additional details in the PR description to make it more comprehensive. Please feel free to review the changes, and if there are any further suggestions or adjustments needed, I'm ready to make them. Your guidance is invaluable in helping me improve my code. I truly appreciate your input and suggestions.

@jhamon jhamon merged commit 4f93203 into pinecone-io:main Sep 23, 2023
2 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants