-
Notifications
You must be signed in to change notification settings - Fork 289
Azure Cosmos multiple stores per container #2953
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
calebschoepp
left a comment
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.
I have very little familiarity with this code, but reading through it it seems plausibly correct!
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
c6d9b1d to
40dfc1e
Compare
Signed-off-by: Ryan Levick <[email protected]>
kate-goldenring
left a comment
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. Love that the enables users to get more out of one cosmos container. It would be nice to add some unit tests, maybe with something like mockall to assert queries and partitions are correctly set.
| pub fn new() -> Self { | ||
| Self::default() | ||
| /// | ||
| /// When `app_id` is provided, the store will a partition key of `$app_id/$store_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.
| /// When `app_id` is provided, the store will a partition key of `$app_id/$store_name`, | |
| /// When `app_id` is provided, the store will be a partition key of `$app_id/$store_name`, |
| if let Some(pk) = partition_key { | ||
| query.push_str(" AND c.partition_key='"); | ||
| query.push_str(pk); | ||
| query.push('\'') |
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.
What is the purpose of this trailing \?
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're pushing single quote (') on to the end of the query (to close the single quote that comes right after the =). The \ escapes the ' character since characters are always themselves enclosed in ''.
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.
I am testing this locally and gets don't seem to be working as expected despite the query being formulated as expected:
SELECT * FROM c WHERE c.id='wow' AND c.partition_key='foo/default'(I hard coded app id into spin to test this out for a default store)
| /// | ||
| /// If the partition key is not set, the store will use `/id` as the partition key. | ||
| partition_key: Option<String>, | ||
| } |
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.
Is there a reason we don't set partition here instead of suffixing the request? Are we anticipating gets across stores?
async fn get_pair(&self, key: &str) -> Result<Option<Pair>, Error> {
let query = self
.client
.query_documents(Query::new(self.get_query(key)))
.query_cross_partition(true)
.max_item_count(1);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.
I'm not sure I understand your question. Can you reformulate it?
kate-goldenring
left a comment
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.
I realized what my issue was. The implementation assumes that the container "Partition Key" is set to /partition_key. You mention this in the description but i skipped over it accidentally. Maybe naming the partition key something else such as "store_identifier" would help.
Signed-off-by: Ryan Levick <[email protected]>
|
I've switched from the |
Fixes #2951
This adds support in the Azure Cosmos key-value implementation for storing multiple key-value stores in a single Azure Cosmos container.
IMPORTANT: This currently does not change the Spin CLI experience at all. Users of Spin CLI continue to have the 1 to 1 relationship between containers and key-value stores. This only allows Spin runtime embedders the ability to have many stores per containers. We can consider changing the default behavior for the Spin CLI in the future, but this might need to be at a major version change.
For embedders who provide an
app_idto theKeyValueAzureCosmos, stores will be created in the supplied container and each key/value pair will include a partition_key field in the form of "$app_id/$store_id".cc @devigned