Skip to content
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

Azure Cosmos multiple stores per container #2953

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Dec 13, 2024

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_id to the KeyValueAzureCosmos, 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

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev requested a review from calebschoepp December 13, 2024 11:44
Copy link
Collaborator

@calebschoepp calebschoepp left a 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!

Ok(Arc::new(AzureCosmosStore {
client: self.client.clone(),
partition_key: self.app_id.as_ref().map(|i| format!("{i}/{name}")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If app_id is None will this incorrectly leave a leading forward slash in the partition key?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see below that it seems like the leading slash is okay, but I'll leave the question here to double check.

Comment on lines +245 to +258
fn get_query(&self) -> String {
let mut query = format!("SELECT * FROM c WHERE c.id='{}'", self.key);
self.append_partition_key(&mut query);
query
}

fn append_partition_key(&self, query: &mut String) {
if let Some(pk) = &self.partition_key {
query.push_str(" AND c.partition_key='");
query.push_str(pk);
query.push('\'')
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these redefined here?

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.

[Azure Cosmos Key-Value] Add support for multiple stores in a single container
2 participants