-
Notifications
You must be signed in to change notification settings - Fork 231
removing tombstones #3013
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
base: v5.3
Are you sure you want to change the base?
removing tombstones #3013
Conversation
Signed-off-by: Steve Hawkins <[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.
Added some comments to fill in some details, refine naming.
But otherwise this looks awesome! thank you!
| return this; | ||
| } | ||
|
|
||
| public Builder<R> parseResourceVersionsForEventFilteringAndCaching(boolean parse) { |
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.
comparableResourceVersions we should probably have this also as withComparableResourceVersions(boolean) that is a more generic 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.
I did this to be consistent with the other configuration for now. I can use a different name to start with here, or we can use a follow-up issue on how to approach the renaming in general.
| private PrimaryToSecondaryMapper<?> primaryToSecondaryMapper; | ||
| private SecondaryToPrimaryMapper<R> secondaryToPrimaryMapper; | ||
| private KubernetesClient kubernetesClient; | ||
| private boolean comparableResourceVersions = true; |
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 should add this also to @KubernetesDependent annotation.
| log.debug("Resource found in cache: {} for id: {}", res.isPresent(), resourceID); | ||
| return res; | ||
| } | ||
| log.debug( |
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.
Would it make sense also to log if parseResourceVersions is true/false? But that might be trivial to see.
|
|
||
| public Optional<String> getLastSyncResourceVersion(Optional<String> namespace) { | ||
| return getSource(namespace.orElse(WATCH_ALL_NAMESPACES)) | ||
| .map(source -> source.getLastSyncResourceVersion()); |
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.
nit: can be replaced by methode reference
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.
Sorry, this can be removed - see #3015
| .getResourceVersion() | ||
| .equals(previousResourceVersion)) | ||
| || isLaterResourceVersion(resourceId, newResource, cachedResource))) { | ||
| // first check against the source in general - this also prevents resurrecting resources when |
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.
Could you pls expand the comment, and explain more in details (maybe sequence of events) how this prevents resurrection ?
Replaces: #2989 to target v5.3
Adds per informer configuration and uses the configuration for the primary resource.
Probably superceded by : #3015 - at the very least some changes from there are needed here.