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

Refactor to improve user-experience and delete support for endpoint construction for data plane operations using projectId, env, and indexName #72

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

rohanshah18
Copy link
Contributor

@rohanshah18 rohanshah18 commented Mar 1, 2024

Problem

Currently the user has to create multiple config objects before they can call data plane operations.

Solution

I have added PineconeConnection() constructor that takes apiKey and indexName as input parameters, and internally creates the config objects to provide a connection object directly without having the need to create the config objects. I have also refactored the classes especially PineconeConnection class as a part of this PR. Lastly, the endpoint construction for gRPC calls using projectName, indexName, and environment is discontinued, and will only use host parameter to get the endpoint. The host parameter is obtained by calling describeIndex().

Refactored the following:

  1. Removed PineconePackageInfo since it wasn't used anywhere
  2. Moved PineconeClient, PineconeConnection, PineconeConnectionConfig, and PineconeClientConfig under configs. I'll eventually like to cut down all dependencies to PineconeClient class and delete it but currently some of the test classes do rely on it and we can take care of it in future PR's. Also I'll like to keep PineconeClient out of clients package since I don't want users to think that it's one of the top level clients that will be useful to them.
  3. Moved PineconeControlPlaneClient, PineconeBlockingDataPlaneClient, and PineconeFutureDataPlaneClient under clients package.
  4. Removed ServerSideTimeout field from PineconeClientConfig since the values were not set correctly.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: (explain here)
    Refactored existing codebase so no new functionalities were added.

Test Plan

Describe specific steps for validating this change.

@rohanshah18 rohanshah18 changed the title Refactor for better user experience Refactor to improve user-experience Mar 4, 2024
@rohanshah18 rohanshah18 requested review from austin-denoble and jhamon and removed request for jhamon and austin-denoble March 4, 2024 16:58
@rohanshah18 rohanshah18 changed the title Refactor to improve user-experience Refactor to improve user-experience and delete support for endpoint construction for data plane operations using projectId, env, and indexName Mar 4, 2024
@rohanshah18 rohanshah18 marked this pull request as ready for review March 4, 2024 17:14
@jhamon jhamon requested a review from aulorbe March 4, 2024 17:52
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

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

Overall I feel like we're trending in the right direction here. I definitely think we should look at removing PineconeClient entirely if that's not something we expect users to interact with. Is our intention to maintain the existing config classes to allow easier backwards compatibility on upgrading to v1.0.0?

I've left some thoughts in PineconeConnection, please let me know if there's anything you'd like to discuss.

@rohanshah18 rohanshah18 merged commit 5d087cc into main Mar 5, 2024
7 of 8 checks passed
@rohanshah18 rohanshah18 deleted the rshah/ux branch March 5, 2024 18:48
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