azure-sdk-for-python icon indicating copy to clipboard operation
azure-sdk-for-python copied to clipboard

Consolidate Container Properties cache in Client Instance

Open bambriz opened this issue 1 year ago • 11 comments

Description

This PR makes the cache for Container Properties (namely Partition Key Definition and RID) for a container to be cached in the client instance instead of the container instance. Because at the moment the Python SDK does not handle caches the same as other SDKs which include refreshing the cache (See Issue 35294) it is not a regression since it is just consolidating the cache of the properties to a single cache in the client instance and the python sdk has not yet implemented caching the container properties as the other SDKs do.

This PR will avoid a situation where: if you call single instances of a container multiple times from a client, it will perform a container read for every operation that does Create, Replace, Upsert, and ReadOffers. It will now instead use the cached container properties after the first container read has already been performed for that specific container.

The Client keeps a cache of properties of each container that is used in the lifetime of the client instance.

The current properties that are cached are "_self" which is the container link "dbsname/coll/containername" , the RID of the container, and the partition key definition of the container.

container_properties = { "_self": "dbsname/colls/containername", "_rid": "########", "partitionKey": {...} }

All SDK Contribution checklist:

  • [ ] The pull request does not introduce [breaking changes]
  • [ ] CHANGELOG is updated for new features, bug fixes or other significant changes.
  • [ ] I have read the contribution guidelines.

General Guidelines and Best Practices

  • [ ] Title of the pull request is clear and informative.
  • [ ] There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • [ ] Pull request includes test coverage for the included changes.

bambriz avatar Apr 20 '24 21:04 bambriz

/azp run python - cosmos - tests

bambriz avatar Apr 20 '24 21:04 bambriz

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 20 '24 21:04 azure-pipelines[bot]

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-cosmos

azure-sdk avatar Apr 20 '24 22:04 azure-sdk

/azp run python - cosmos - tests

bambriz avatar Apr 21 '24 01:04 bambriz

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 21 '24 01:04 azure-pipelines[bot]

/azp run python - cosmos - tests

bambriz avatar Apr 21 '24 07:04 bambriz

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 21 '24 07:04 azure-pipelines[bot]

/azp run python - cosmos - tests

bambriz avatar Apr 29 '24 21:04 bambriz

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 29 '24 21:04 azure-pipelines[bot]

/azp run python - cosmos - tests

bambriz avatar Apr 30 '24 17:04 bambriz

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 30 '24 17:04 azure-pipelines[bot]

Thanks for the changes @bambriz. Overall, they look good to me. I have 2 concerns:

  1. We should add tests which also make sure the cache is populated correctly, i.e. with correct properties like rid and partition key definition, etc.
  2. Currently it looks like the container_properties_cache is public, which should not be the case. AFAIK, python doesn't have private variables, but at least we can use naming conventions to make it private?

Access to the properties cache will be needed outside of the cosmos client connection class, so we would need protected setters and (Read only) getters for the cache. I'll go ahead and make those changes.

bambriz avatar May 07 '24 23:05 bambriz