-
Notifications
You must be signed in to change notification settings - Fork 33
DOCSP-47797 Add VS and AVS to indexes page #462
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
Conversation
✅ Deploy Preview for docs-golang ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
"go.mongodb.org/mongo-driver/bson" | ||
"go.mongodb.org/mongo-driver/mongo" | ||
"go.mongodb.org/mongo-driver/mongo/options" |
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.
We'll need a version that is compatible with v2 as well (note that there were breaking changes in v2, so there may be additional changes needed).
"go.mongodb.org/mongo-driver/bson" | |
"go.mongodb.org/mongo-driver/mongo" | |
"go.mongodb.org/mongo-driver/mongo/options" | |
"go.mongodb.org/mongo-driver/v2/bson" | |
"go.mongodb.org/mongo-driver/v2/mongo" | |
"go.mongodb.org/mongo-driver/v2/mongo/options" |
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.
Thanks, just edited! All that seemed to break was that the context
was no longer needed in mongo.Connect()
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.
LGTM! A few nits and comments, looks great!
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
// end-list-index | ||
|
||
// start-update-index | ||
// Specify the index name and the new index definition |
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.
// Specify the index name and the new index definition | |
// Specifies the index name and the new index definition |
Nit: Keeps it in the same tense as the rest of the comments
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.
@shuangela According to this guidance on code comments, comments that indicate that a user needs to take an action should be imperative. For this section, the user needs to replace the index name and definition, so the imperative tense is necessary here.
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.
Was going to make the same comment as @shuangela and then saw your response @lindseymoore. We can keep the imperative if that's the guidance, but is there a way we can make it even clearer to the user that they should modify the code here?
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.
Since it's an example, I propose we just use the same index name as in the previous examples. As part of this, I would change all of the examples to use the same index name consistently:
const indexName = "atlas_vector_search_index"
const indexName = "atlas_search_index"
or:
const indexName = "vector_search_index"
const indexName = "search_index"
And then we would end up with something like this:
// Specifies the index name and the new index definition
const indexName = "search_index"
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.
On second thought, do some of the examples apply to both AS and AVS?
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.
Yes, the delete, list, and update methods are used the same for both AS and AVS. I updated the examples to remove the placeholders and include more general names, like myIndex. The only exception is in the update section where I made the name vector_index, since the definition shown is for a vector index.
Also changed the comments back to "specifies".
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.
Ok, that makes sense. Thanks for making the change.
Any reason you decided to not make the names atlas_search_index
and vector_index
more consistent though? I would either use "atlas" for both or neither. Also, the corresponding terms are "search" vs "vector search" rather than "search" vs "vector". Let me know if there's something I'm missing
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.
Thanks for clarifying! Updated names to vector_search_index
and search_index
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
57b82ab
to
c11990e
Compare
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
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.
A few questions/comments
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
// end-list-index | ||
|
||
// start-update-index | ||
// Specify the index name and the new index definition |
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.
On second thought, do some of the examples apply to both AS and AVS?
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.
Just a couple more comments
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
source/includes/fundamentals/code-snippets/indexes/atlasVectorSearch.go
Outdated
Show resolved
Hide resolved
// end-list-index | ||
|
||
// start-update-index | ||
// Specify the index name and the new index definition |
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.
Ok, that makes sense. Thanks for making the change.
Any reason you decided to not make the names atlas_search_index
and vector_index
more consistent though? I would either use "atlas" for both or neither. Also, the corresponding terms are "search" vs "vector search" rather than "search" vs "vector". Let me know if there's something I'm missing
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.
1 comment for a check error.
2 comments for whether we should use "search" vs "Atlas Search and/or Atlas Vector Search".
source/fundamentals/indexes.txt
Outdated
Delete Search Indexes | ||
````````````````````` | ||
|
||
The following example deletes a search index with the specified name: |
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.
"a search index" would refer to both Atlas Search and Atlas Vector Search. If we'd like to say Atlas Search, then we should mention Atlas Vector Search as well, since the example applies to both.
The following example deletes a search index with the specified name: | |
The following example deletes an Atlas Search index or an Atlas Vector Search index with the specified name: |
or
The following example deletes a search index with the specified name: | |
The following example deletes an Atlas Search or Atlas Vector Search index with the specified name: |
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.
LGTM! Thank you, Lindsey
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v2.1 v2.1
# Navigate to the new working tree
cd .worktrees/backport-v2.1
# Create a new branch
git switch --create backport-462-to-v2.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fe0aa5b98a77cb4a3b9557efd92b0741821bb763
# Push it to GitHub
git push --set-upstream origin backport-462-to-v2.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v2.1 Then, create a pull request where the |
Pull Request Info
PR Reviewing Guidelines
JIRA - https://jira.mongodb.org/browse/DOCSP-47797
Staging - https://deploy-preview-462--docs-golang.netlify.app/fundamentals/indexes/#atlas-search-and-vector-search-indexes
Self-Review Checklist