Add ConfigureIndex method #34

aulorbe · 2024-07-01T18:27:20Z

Problem

To have parity with our other clients, we need to include a ConfigureIndex method on the Pinecone client.

Asana ticket.

Solution

Add a ConfigureIndex method. Users can pass a pod size, a # of replicas, or both.

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)

Test Plan

New integration tests confirm functionality + pass in CI.

pinecone/client_test.go

austin-denoble

Nice work, looks pretty good overall. I've got a few comments and some feedback for your consideration.

austin-denoble · 2024-07-10T17:46:29Z

pinecone/client.go

+// Example for a pods-based index originally configured with 1 "p1" pod of size "x2" and 1 replica:
+//  // To scale the size of your pods from "x2" to "x4":
+//   _, err := pc.ConfigureIndex(ctx, "my-index", "p1.x4", nil)
+//   if err != nil {
+//       fmt.Printf("Failed to configure index: %v\n", err)
+//   }
+//
+//  // To scale the number of replicas:
+//   _, err := pc.ConfigureIndex(ctx, "my-index", nil, 4)
+//   if err != nil {
+//       fmt.Printf("Failed to configure index: %v\n", err)
+//   }
+//
+//  // To scale both the size of your pods and the number of replicas:
+//   _, err := pc.ConfigureIndex(ctx, "my-index", "p1.x4", 4)
+//   if err != nil {
+//       fmt.Printf("Failed to configure index: %v\n", err)
+//   }
+//


Did you happen to run gofmt on this file? Just wondering as the formatting seems a bit off. There should be a blank empty line between the "Example for a pods-based index..." line and the first comment of the code block.

I think gofmt should also indent the code block further, but I'm not positive.

Ah I didn't -- this was pushed before before our Slack convo about gofmt. I'll run it!

Okay done :)

austin-denoble · 2024-07-10T17:49:37Z

pinecone/client.go

@@ -273,6 +273,104 @@ func (c *Client) DeleteIndex(ctx context.Context, idxName string) error {
 	return nil
 }

+// ConfigureIndex is used to [scale a pods-based index] up or down by changing the size of the pods or the number of
+//replicas.


Looks like a space is needed between // and replicas.

austin-denoble · 2024-07-10T17:50:24Z

pinecone/client.go

+//
+// Parameters:
+//  - name: The name of the index to configure.
+//  - pods: (Optional) The pod size to scale the index to (e.g. for a "p1" pod type,


I like this formatting for (Optional). I didn't do the same thing in other areas but we can maybe do another doc comment cleanup pass.

Yeah, sweet!

austin-denoble · 2024-07-10T18:01:20Z

pinecone/client.go

+//
+// [scale a pods-based index]: https://docs.pinecone.io/guides/indexes/configure-pod-based-indexes
+// [app.pinecone.io]: https://app.pinecone.io
+func (c *Client) ConfigureIndex(ctx context.Context, name string, pods *string,


pods may be a little confusing given that fields means something else when creating an index. Although I'm realizing now we don't provide a pods value in CreatePodIndexRequest.

Still, we use PodType there and I think we should do the same here. I know it's a positional arg, but just for clarity in the code here. I know we then assign things to podType further down, but initially when I was reading the function I was a bit confused.

Okay, np, I'll change to podType! (I hate how we call it type b/c technically it's size, but yeah best not to confuse ppl who're used to our other SDKs)

austin-denoble · 2024-07-10T18:35:06Z

pinecone/client.go

+		err := handleErrorResponseBody(req, "failed to configure index: ")
+		if err != nil {
+			return nil, err
+		}


I think here you can just do:

Suggested change

err := handleErrorResponseBody(req, "failed to configure index: ")

if err != nil {

return nil, err

}

return nil, handleErrorResponseBody(req, "failed to configure index: ")

We can assume if the StatusCode is not http.StatusOK we're returning an error.

austin-denoble · 2024-07-10T18:44:18Z

pinecone/client.go

+		},
+	}
+
+	req, err := c.restClient.ConfigureIndex(ctx, name, request)


Can we rename req to res to make it clearer it's the http.Response from the function call here?

austin-denoble · 2024-07-10T18:50:08Z

pinecone/client.go

+		return nil, err
+	}
+
+	defer req.Body.Close()


nit: In other functions we put this line right after the function call so it's a bit more clear we're actually closing it, so I'd maybe move it up to after the initial function call and error check:

req, err := c.restClient.ConfigureIndex(ctx, name, request) if err != nil { log.Fatalf("Failed to configure index %s. Error: %v", name, err) } defer req.Body.Close()

okay will do!

austin-denoble · 2024-07-10T18:50:47Z

pinecone/client_test.go

+	"github.com/google/uuid"
+	"github.com/pinecone-io/go-pinecone/internal/mocks"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
 	"io"
+	"log"
 	"net/http"
 	"os"
 	"reflect"
 	"strings"
 	"testing"
-
-	"github.com/google/uuid"
-	"github.com/pinecone-io/go-pinecone/internal/mocks"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"github.com/stretchr/testify/suite"


nit: I think goimports should split these apart automatically right?

I thinkkk so. This was also pre-our formatting convo, so I'll push up some changes wrt that soon!

austin-denoble · 2024-07-10T18:59:32Z

pinecone/client.go

+		}
+	}
+
+	response, err := control.ParseConfigureIndexResponse(req) // TODO: need this?


I don't think we should be using the internals, I don't think we're doing that anywhere else. I'd maybe look at how we're handling things in something like DescribeIndex. Could you juse use the decodeIndex function instead?

Suggested change

response, err := control.ParseConfigureIndexResponse(req) // TODO: need this?

res, err := c.restClient.ConfigureIndex(ctx, name, request)

if err != nil {

log.Fatalf("Failed to configure index %s. Error: %v", name, err)

}

defer res.Body.Close()

if req.StatusCode != http.StatusOK {

return nil, handleErrorResponseBody(req, "failed to configure index: ")

}

return decodeIndex(res.Body)

Ultimately this just returns an IndexModel, there's not really anything else in ConfigureIndexResponse, so you'd need to update the return type and maybe doc string here.

Sure yeah good call, I'll do that!

aulorbe · 2024-07-12T15:53:57Z

pinecone/client.go

+
+	defer res.Body.Close()
+
+	if res.StatusCode != http.StatusOK {


Changed this from our usualhttp.StatusCreated to http.StatusOK, since configuring an index doesn't give status created or whatnot. Just fyi!

austin-denoble

Looks good, nice work. Thanks for iterating as well! 🚢

aulorbe added 8 commits July 1, 2024 11:27

Add ConfigureIndex method

ce920dc

Not sure if this is right, but it is working

1b6c867

Finishing touches

366b4bc

Make test indexes

1b58b5f

Add new secret to ci yaml

89e3368

Think cache has cleared now

7fb2f0c

test out deleting, creating, deleting

1b49970

Trying again without log.Fatalf

018feb0

aulorbe commented Jul 8, 2024

View reviewed changes

pinecone/client_test.go Outdated Show resolved Hide resolved

aulorbe added 3 commits July 8, 2024 15:28

Get rid of single name index

21ce7d6

Remove secret from CI + rename erdr to err

a99a56f

Clean up docstring

75713e5

aulorbe requested a review from austin-denoble July 8, 2024 22:40

aulorbe marked this pull request as ready for review July 8, 2024 22:40

Lint

70e0bf2

austin-denoble reviewed Jul 10, 2024

View reviewed changes

aulorbe added 7 commits July 10, 2024 12:38

Merge branch 'main' into Audrey/configure-index

0536f5c

Run gofmt and goimports

cb6b716

Rename vars, clean up logic

f53f536

Address final comments

adc7e3a

Fix ConfigureIndex error handling

f3939ed

Oops, remove debugging line

a3049bd

Change to StatusOk b/c not checking if index has been created

1046619

aulorbe requested a review from austin-denoble July 12, 2024 15:52

aulorbe commented Jul 12, 2024

View reviewed changes

lint

6769041

austin-denoble approved these changes Jul 12, 2024

View reviewed changes

aulorbe merged commit cef7a46 into main Jul 12, 2024
3 checks passed

aulorbe deleted the Audrey/configure-index branch July 12, 2024 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ConfigureIndex method #34

Add ConfigureIndex method #34

aulorbe commented Jul 1, 2024 •

edited

Loading

austin-denoble left a comment

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

austin-denoble Jul 10, 2024

aulorbe Jul 10, 2024

aulorbe Jul 12, 2024

austin-denoble left a comment

-	response, err := control.ParseConfigureIndexResponse(req) // TODO: need this?
+	res, err := c.restClient.ConfigureIndex(ctx, name, request)
+		if err != nil {
+		log.Fatalf("Failed to configure index %s. Error: %v", name, err)
+	}
+	defer res.Body.Close()
+	if req.StatusCode != http.StatusOK {
+	    return nil, handleErrorResponseBody(req, "failed to configure index: ")
+	}
+	return decodeIndex(res.Body)

Add ConfigureIndex method #34

Add ConfigureIndex method #34

Conversation

aulorbe commented Jul 1, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

aulorbe commented Jul 1, 2024 •

edited

Loading