-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update index operations #62
Conversation
src/integration/java/io/pinecone/integration/controlPlane/index/pod/ConfigureIndexTest.java
Show resolved
Hide resolved
src/integration/java/io/pinecone/integration/dataplane/UpdateAndQueryTest.java
Show resolved
Hide resolved
...pinecone/integration/controlPlane/index/serverless/CreateDescribeListAndDeleteIndexTest.java
Show resolved
Hide resolved
...ration/java/io/pinecone/integration/controlPlane/index/pod/CreateListAndDeleteIndexTest.java
Show resolved
Hide resolved
@austin-denoble Overall, this change will unblock you with adding collections and more refactoring are possible in future as we discussed internally. But any of the classes you're dealing with, are finalized as a part of this PR. |
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.
Nice job, overall this looks good to me as a first step towards refactoring everything around the generated code, I think will unblock us to continue with the integration tests and then data plane refactoring.
Main concerns for me right now are making sure we make the UX reasonable for users. I think there's a bunch we could do to refactor PineconeClient
, PineconeClientConfig
, PineconeConnection
, and PineconeConnectionConfig
into something more reasonable, because it feels like a lot to keep track of right now.
In the other clients, we have a top-level Pinecone
class that allows you to perform control plane operations, and then that class abstracts a lot of the internals for hitting the dataplane, so users can more easily target and work with indexes without a bunch of setup. I think we should try and shoot for something similar in Java, does that make sense? We can discuss further offline.
Problem
After adding open api generated code for control plane, there's a need for a wrapper for the index operations.
Solution
This PR adds a wrapper for index operations and associated integration tests. Couple of the integration tests are currently commented out and will be handled in the upcoming PR's.
Type of Change
Test Plan
Ran integration tests.