-
Notifications
You must be signed in to change notification settings - Fork 260
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
Conversation
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]>
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)
/// An optional partition key to use for all operations. | ||
/// | ||
/// 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?
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_id
to 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