Implement deletion protection #40

ericapywang · 2024-07-24T16:39:00Z

Problem

Deletion protection was not implemented.

Solution

We updated the OpenAPI code to include deletion protection. This changed the parameters for several structs and functions, so all this code was updated to reflect the changes.

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

Test cases should still pass.

pinecone_sdk/src/pinecone/control.rs

emily-emily · 2024-07-29T15:23:19Z

pinecone_sdk/src/pinecone/control.rs

-    /// It applies to pod-based indexes only.
-    /// Serverless indexes scale automatically based on usage.
+    /// This operation changes the deletion protection specification, the pod type, and the number of replicas for an index.
+    /// Deletion protection can be changed for both pod and serverless indexes, while pod types and number of replicas can only be changed for pod indexes.


I wonder if it would be easier to use if we split this into pod and serverless versions?

That's a good thought. We didn't take this approach for configure_index in any of the other clients, but we did split the create function into separate functions for pod / serverless in Go and Java, and you all did the same in Rust.

I think leaving it as a single function is probably fine for now, and aligns with the other SDKs. If later on we decide to expose more specific configure functions in any of these clients, those could be added additively while maintaining these original base functions.

emily-emily · 2024-07-29T15:26:08Z

pinecone_sdk/src/pinecone/control.rs

@@ -901,6 +925,7 @@ mod tests {
                state: openapi::models::index_model_status::State::Ready,
            }),
            host: "mock-host".to_string(),
+            deletion_protection: None,


What does deletion_protection None mean? Would it be confusing to pass DeletionProtection to other functions and then get Option<DeletionProtection> in the returned IndexModel?

I may be wrong, but I think this is because the describe_index endpoint doesn't return a deletion_protection field in its response, so IndexModel will not always include it. I agree that it's confusing though, but I'm not too sure how to get around it.

describe_index should be returning deletion_protection, are you passing the X-Pinecone-Api-Version header with 2024-07?

Here's an example of curl'ing with the header:

curl -X GET https://api.pinecone.io/indexes/test-configure-index -H "Api-Key: b56c33c1-ba11-4f93-9333-4235ffdaac43" --header "X-Pinecone-Api-Version: 2024-07" {"name":"test-configure-index","metric":"cosine","dimension":3,"status":{"ready":true,"state":"Ready"},"host":"test-configure-index-1390950.svc.apw5-4e34-81fa.pinecone.io","spec":{"serverless":{"region":"us-west-2","cloud":"aws"}},"deletion_protection":"enabled"}%

If the header isn't provided, the deletion_protection field is not returned. I believe it's actually marked as an optional field in the OpenAPI spec though, so I think None would be synonymous with "disabled".

Makes sense! I just copied it from the API docs, which didn't seem to include a deletion_protection field

austin-denoble

Overall looking good. I had some feedback around making deletion_protection an Option in most cases. Primarily, because it's not represented as required in the OpenAPI spec, and we treat it as optional in the other clients generally when creating indexes or configuring them. It may be nice to take the same approach here if possible.

I saw you also mentioned not seeing deletion_protection in describe_index responses - I'd make sure the X-Pinecone-Api-Version header is being properly set for outgoing requests, that's most likely why.

austin-denoble · 2024-07-29T16:27:51Z

pinecone_sdk/src/pinecone/control.rs

-    /// It applies to pod-based indexes only.
-    /// Serverless indexes scale automatically based on usage.
+    /// This operation changes the deletion protection specification, the pod type, and the number of replicas for an index.
+    /// Deletion protection can be changed for both pod and serverless indexes, while pod types and number of replicas can only be changed for pod indexes.


That's a good thought. We didn't take this approach for configure_index in any of the other clients, but we did split the create function into separate functions for pod / serverless in Go and Java, and you all did the same in Rust.

I think leaving it as a single function is probably fine for now, and aligns with the other SDKs. If later on we decide to expose more specific configure functions in any of these clients, those could be added additively while maintaining these original base functions.

austin-denoble · 2024-07-29T16:38:40Z

pinecone_sdk/src/pinecone/control.rs

    /// # Ok(())
    /// # }
    /// ```
    pub async fn configure_index(
        &self,
        name: &str,
-        replicas: i32,
-        pod_type: &str,
+        deletion_protection: DeletionProtection,


I actually think deletion_protection should also be an option here: Option<DeletionProtection>.

If you don't send the value along with the API request, the server will ignore changing it which is the behavior I think we'd expect here. We probably don't want users that are configuring a pod index to need to know which DeletionProtection value the index was configured with in order to make this request.

In the Go client I went with making all 3 of the fields optional, but I do a small amount of client-side validation to make sure the user supplied at least one argument: https://github.com/pinecone-io/go-pinecone/blob/main/pinecone/client.go#L854-L857

austin-denoble · 2024-07-29T16:54:22Z

pinecone_sdk/src/pinecone/control.rs

@@ -901,6 +925,7 @@ mod tests {
                state: openapi::models::index_model_status::State::Ready,
            }),
            host: "mock-host".to_string(),
+            deletion_protection: None,


describe_index should be returning deletion_protection, are you passing the X-Pinecone-Api-Version header with 2024-07?

Here's an example of curl'ing with the header:

curl -X GET https://api.pinecone.io/indexes/test-configure-index -H "Api-Key: b56c33c1-ba11-4f93-9333-4235ffdaac43" --header "X-Pinecone-Api-Version: 2024-07" {"name":"test-configure-index","metric":"cosine","dimension":3,"status":{"ready":true,"state":"Ready"},"host":"test-configure-index-1390950.svc.apw5-4e34-81fa.pinecone.io","spec":{"serverless":{"region":"us-west-2","cloud":"aws"}},"deletion_protection":"enabled"}%

If the header isn't provided, the deletion_protection field is not returned. I believe it's actually marked as an optional field in the OpenAPI spec though, so I think None would be synonymous with "disabled".

austin-denoble · 2024-07-29T16:56:48Z

pinecone_sdk/src/pinecone/control.rs

@@ -72,6 +74,7 @@ impl PineconeClient {
        metric: Metric,
        cloud: Cloud,
        region: &str,
+        deletion_protection: DeletionProtection,


Would it make sense to make this an Option? If it's not provided, it defaults to disabled, but I'm not sure what makes idiomatic sense for Rust here.

I'm inclined to leave it as not an Option, since the DeletionProtection enum implements the Default trait, so the user can just call Default::default() to have it default to disabled. Would love to hear other thoughts though!

austin-denoble · 2024-07-29T17:05:34Z

pinecone_sdk/tests/integration_test_control.rs

        .await
        .expect("Failed to configure index");

    Ok(())
 }

+#[tokio::test]
+async fn test_configure_deletion_protection() -> Result<(), PineconeError> {


Nice test coverage. 👍

If you end up making deletion_protection an Option on configure_index, you may also want to write a test to validate the scenario where you have an index with deletion_protection: "enabled", and you send a configure_index request to configure something other than the deletion_protection value, we want to make sure it's maintained.

Jen had run into an issue where the Python client was defaulting the outgoing deletion_protection value to disabled for configure_index requests where it wasn't explicitly set by the user.

austin-denoble

Looks good, thanks for iterating!

## Problem Deletion protection was not implemented. ## Solution We updated the OpenAPI code to include deletion protection. This changed the parameters for several structs and functions, so all this code was updated to reflect the changes. ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] 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 Test cases should still pass. --------- Co-authored-by: Emily Yu <emily.y@pinecone.io>

emily-emily and others added 7 commits July 24, 2024 12:05

Regenerate openapi code

3cec3fa

add deletion protection parameters

2c72489

reformatted tests

5a33d48

failing test

02c8797

fix deletion protection

4713f60

update documentation

9d9860b

Merge branch 'main' into emily/regenerate-code

e9c20a9

ericapywang marked this pull request as ready for review July 26, 2024 13:20

ericapywang requested review from austin-denoble, emily-emily, haruska and ssmith-pc July 26, 2024 13:47

ericapywang added 3 commits July 26, 2024 10:20

remove commented out lines

c021065

Merge branch 'main' into emily/regenerate-code

681f36c

Merge branch 'main' into emily/regenerate-code

0603413

emily-emily reviewed Jul 29, 2024

View reviewed changes

ericapywang added 3 commits July 29, 2024 11:51

remove default in docs

15b161c

fixed docs

553f1cf

add configure index unit test

76fb594

austin-denoble reviewed Jul 29, 2024

View reviewed changes

ericapywang and others added 3 commits July 29, 2024 13:27

add delete_protection to unit test

2fe6a42

make deletion protection an option

838b4aa

Merge branch 'main' into emily/regenerate-code

64a8b9e

emily-emily force-pushed the emily/regenerate-code branch from f5bf76a to 64a8b9e Compare July 29, 2024 18:17

emily-emily and others added 2 commits July 29, 2024 14:19

Remove root/openapi folder

b726bae

added test case + error type

c5eb27d

ericapywang requested review from austin-denoble and emily-emily July 29, 2024 18:50

ericapywang added 2 commits July 29, 2024 17:20

add checks to ensure some parameter is passed for configure index

97e5255

add test case

3474bff

ericapywang added 2 commits July 29, 2024 17:57

tag inference unit test

41842a6

remove test tag

9c45089

austin-denoble approved these changes Jul 30, 2024

View reviewed changes

ericapywang merged commit b791291 into main Jul 30, 2024
1 check passed

ericapywang deleted the emily/regenerate-code branch July 30, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement deletion protection #40

Implement deletion protection #40

ericapywang commented Jul 24, 2024

emily-emily Jul 29, 2024

austin-denoble Jul 29, 2024

emily-emily Jul 29, 2024

ericapywang Jul 29, 2024 •

edited

Loading

austin-denoble Jul 29, 2024

ericapywang Jul 29, 2024

austin-denoble left a comment

austin-denoble Jul 29, 2024

austin-denoble Jul 29, 2024

austin-denoble Jul 29, 2024

austin-denoble Jul 29, 2024

ericapywang Jul 29, 2024

austin-denoble Jul 29, 2024

austin-denoble left a comment

Implement deletion protection #40

Implement deletion protection #40

Conversation

ericapywang commented Jul 24, 2024

Problem

Solution

Type of Change

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericapywang Jul 29, 2024 • edited Loading

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

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

ericapywang Jul 29, 2024 •

edited

Loading