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

Bulk executor cache #857

Merged
merged 11 commits into from
Oct 3, 2019
Merged

Bulk executor cache #857

merged 11 commits into from
Oct 3, 2019

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Sep 27, 2019

Pull Request Template

Description

Currently we are maintaining the Executor instance as part of the Container instance, but the Container instance is really too ephemereal and users could be calling GetContainer multiple times for the same container across their code, in which case we would end up creating a lot of Executor instances that would not be used or worse, the desired performance effect would not be reached if the concurrent operations are using different Container instances for the same container name.

This PR introduces a cache at the Client level to have one Executor per Container based on the Link. This way, Executor instances can be shared across Container instances representing the same container and hence, achieving the desired performance benefit even when the user is calling GetContainer from concurrent threads.

The Executor cache is maintained in sync when DeleteContainer operations are issued, by removing cache entries for deleted containers.

The cache is also tied to the CosmosClient lifetime, when the client is disposed, the Executor cache and instances get disposed.

@kirankumarkolli
Copy link
Member

One more alternative is to include Executor at Partition cache.
PartitionCache is global and we just need to ensure refreshed doesn't throw away existing ones.
It might need to interact with HA layer.

On similar notes CollectionCache can also be leveraged. Its very specific to SDK code.
Thoughts?

@kirankumarkolli
Copy link
Member

kirankumarkolli commented Sep 27, 2019

Update from offline sync-up:

  1. Extending existing stateless caches is non-trivial -> new cache is good
  2. Bulk needs to cover Gone with NameCacheStale in Retry Policies
  3. Prefer AsyncCache< Executors>

Copy link
Member

@kirankumarkolli kirankumarkolli left a comment

Choose a reason for hiding this comment

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

Extending existing stateless caches is non-trivial -> new cache is good
Bulk needs to cover Gone with NameCacheStale in Retry Policies
Prefer AsyncCache< Executors>

@ealsur
Copy link
Member Author

ealsur commented Sep 27, 2019

@kirankumarkolli Retry policy for NameStaleCache is a separate PR, see #859

Updated PR to use AsyncCache

Copy link
Member

@kirankumarkolli kirankumarkolli left a comment

Choose a reason for hiding this comment

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

No contract change but contract file touched.

@kirankumarkolli
Copy link
Member

Its worth a mention in release notes.

@kirankumarkolli kirankumarkolli merged commit efc5c78 into master Oct 3, 2019
@kirankumarkolli kirankumarkolli deleted the users/ealsur/bulkshared branch October 3, 2019 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants