refactor: remove the use of knownResourceVersions #2985
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the TemporaryResourceCache class by removing the explicit tracking of known resource versions through the knownResourceVersions field and instead uses existing cache checks to determine if a resource version is known.
- Removed
knownResourceVersionsfield and related initialization logic - Simplified the
isKnownResourceVersionmethod to use existing cached resources and tombstones for version checking - Eliminated the need to explicitly track known versions during resource updates
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return tombstones.contains(resource.getMetadata().getUid()); | ||
| } | ||
|
|
||
| return !isLaterResourceVersion(resourceId, resource, cachedResource); |
There was a problem hiding this comment.
The logic in isKnownResourceVersion has been inverted from checking if a version is in a known set to checking if it's NOT a later version. This semantic change could be confusing and may not be equivalent to the original behavior, especially for edge cases where versions are equal or incomparable.
| return !isLaterResourceVersion(resourceId, resource, cachedResource); | |
| return resource.getMetadata().getResourceVersion() | |
| .equals(cachedResource.getMetadata().getResourceVersion()); |
Copilot uses AI. Check for mistakes.
shawkins
force-pushed
the
iss2984
branch
4 times, most recently
from
f56d852 to
c04c09c
Compare
closes: operator-framework#2984 Signed-off-by: Steve Hawkins <[email protected]>
|
checking the KEP: THey are actually not changing anything, just adding utility and tests, so we are fine with this IMHO, I will also do some related PRs. |
|
@shawkins could you pls rather target |
2 participants
closes: #2984
After seeing that comparable version support is coming in #2981 (comment) I reviewed the current logic and decided that it could be simplified.
We don't need to explicitly track known versions as a check of whether it's an unknown later version will work just as well.
A reason not to do this is if locking is not used, and you want to react to an older version that may have contained competing changes. However that doesn't seem like a valid thing to do from an eventual consistency perspective.