most recent provider and provider stores #42
Conversation
…and version explosion
| provider = self._cache.get(version) | ||
| except KeyError: | ||
| try: | ||
| provider = self._provider_store.provider(self._material_name, version) |
There was a problem hiding this comment.
This version only works with a single material name. While this is the default in the Java one, this behavior can be override in Java. Can this class be modified (possibly post launch) to support this additional functionality?
Please see this commit for additional context: aws/aws-dynamodb-encryption-java@5045e9f#diff-61178aa9fd65f26315b84815e46913fb
|
|
||
| return TtlActions.EXPIRED | ||
|
|
||
| def _can_use_current(self): |
There was a problem hiding this comment.
Why doesn't this delegate to _ttl_action?
There was a problem hiding this comment.
Because I forgot to remove that method when I replaced it with _ttl_action :/
Removing
|
|
||
| try: | ||
| version = self._get_max_version() | ||
| provider = self._get_provider(version) |
There was a problem hiding this comment.
If old version is the same as new version, this still hits DynamoDB (including decryption costs) rather than just continuing to use the version in the cache.
There was a problem hiding this comment.
Fair point. Adding a cache attempt after getting the max version.
|
|
||
| actual_version, provider = self._get_most_recent_version(allow_local) | ||
|
|
||
| self._cache.put(actual_version, provider) |
There was a problem hiding this comment.
Is it an issue that this (and the next line) are not done in the same lock as was acquired in _get_most_recent_version?
In general, I'm worried about this being split across multiple lock calls. It seems like this my result in multiple threads calling DynamoDB for a single refresh (when it isn't necessary).
There was a problem hiding this comment.
Hmm, that's a good point. I had originally split it up for simplicity, but with how the locking has changed since then I agree, this could create unintended side effects.
…e provider store * move local version, last updated, and cache update into _get_most_recent_version to keep them inside the same lock acquisition
Description of changes:
Introduction of
MostRecentProvider, provider stores, andMetaStore.BLOCKED BY #46
NOTE
To ease parallelization of efforts, this PR is based on the
DELETE-9fork. Prior to looking over this PR, #46 should be merged tomasterand the base for this PR be changed tomaster.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.