Skip to content

Add index tags #169

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

Merged
merged 5 commits into from
Jan 10, 2025
Merged

Add index tags #169

merged 5 commits into from
Jan 10, 2025

Conversation

rohanshah18
Copy link
Contributor

@rohanshah18 rohanshah18 commented Dec 19, 2024

Problem

Add support for Index tags.

Solution

Index tags can be passed as a Map<String, String> to the following operations:

  1. create serverless index
  2. create pod Index
  3. configure serverless index
  4. configure pod index

When configuring tags, following are the special cases:

  1. If an empty map is passed as an input, the tags will remain unchanged
  2. If an empty value is passed as an input, the tag will be removed

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

Updated integration tests for all 4 operations.

@@ -70,13 +69,6 @@ public void testSyncListEndpoint() throws InterruptedException {
listResponseWithPaginationNoPrefix1.getPagination().getNext()
);
assertEquals(listResponseWithPaginationNoPrefix2.getVectorsList().size(), 2);
ListResponse listResponseWithPaginationNoPrefix3 = indexConnection.list(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove extra checks that are failing consistently in integration tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are they failing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The total number of vectors in the custom namespace ("example-namespace") is 4. When listResponseWithPaginationNoPrefix1 returns 2 vectors and includes a pagination token pointing to the remaining vector IDs, these are then returned as part of listResponseWithPaginationNoPrefix2, which has a limit of 2. Since there are no more vector IDs left, no pagination token will be returned in the response of listResponseWithPaginationNoPrefix2. As a result, the assertion on line #79 will fail with the error message: "Pagination token cannot be null or empty." Similarly, this issue will also occur in the asynchronous test.

@rohanshah18 rohanshah18 marked this pull request as ready for review January 9, 2025 15:00
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.

LGTM, just left a few comments. Nice work!

// Create serverless index with deletion protection enabled
controlPlaneClient.createServerlessIndex(indexName, "cosine", 3, "aws", "us-west-2", DeletionProtection.ENABLED);
controlPlaneClient.createServerlessIndex(indexName, "cosine", 3, "aws", "us-west-2", DeletionProtection.ENABLED, tags);
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to add an assertion for the tags that are applied to the index since we're doing that for DeletionProtection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added the additional assertions

@@ -70,13 +69,6 @@ public void testSyncListEndpoint() throws InterruptedException {
listResponseWithPaginationNoPrefix1.getPagination().getNext()
);
assertEquals(listResponseWithPaginationNoPrefix2.getVectorsList().size(), 2);
ListResponse listResponseWithPaginationNoPrefix3 = indexConnection.list(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are they failing?

@rohanshah18 rohanshah18 merged commit 6e109e3 into main Jan 10, 2025
8 of 9 checks passed
@rohanshah18 rohanshah18 deleted the rshah/indextags branch January 10, 2025 17:59
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