removing tombstones #2989
removing tombstones #2989
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
shawkins
force-pushed
the
tombstone_removal
branch
from
fd5a12c to
91cec06
Compare
|
|
||
| boolean moveAhead = false; | ||
| String latest = managedInformerEventSource.getLastSyncResourceVersion(resourceId.getNamespace()).orElse(null); | ||
| if (latest != null && latest > newResource.getMetadata().getResourceVersion()) { |
There was a problem hiding this comment.
Is it guaranteed that those events (maybe for different resource names) come in order in watches?
There was a problem hiding this comment.
But I guess if we going to compare the resource versions in informers that does not matter.
There was a problem hiding this comment.
My understanding is that ordering is guarenteed. So this is applicable not only to getting rid of tombstones, but putting anything into the cache in general - added that as another simplification.
Also the javadoc on getLastSyncResourceVersion could use some improvement. Updates to the cache are made prior to async event processing. So the underlying cache state should be correct up to the given resourceVersion at the time we check getLastSyncResourceVersion.
There was a problem hiding this comment.
My understanding is that ordering is guarenteed. So this is applicable not only to getting rid of tombstones, but putting anything into the cache in general - added that as another simplification.
I see, I know that it is true for a single resource, but for example there are multiple resources in the same type, if that still comes in order, but I guess yes, can also quickly take a look today on some cluster and watch some events.
Thank you, this is great!!
There was a problem hiding this comment.
( also if you could join us tomorrow on community meeting we could discuss strategy would be great, thinking if these improvements should go to 5.2 or rather the next 5.3 so we can iterate and discuss more, and there is no pressure, since might be good to do a minor release soonish )
There was a problem hiding this comment.
Is that on zoom at 15:00 CEST?
shawkins
force-pushed
the
tombstone_removal
branch
from
78af174 to
a810ac9
Compare
| "Temporarily moving ahead to target version {} for resource id: {}", | ||
| newResource.getMetadata().getResourceVersion(), | ||
| resourceId); | ||
| cache.put(resourceId, newResource); |
There was a problem hiding this comment.
This should be safe to do without further conditions as long as there is no chance that the operator sdk is making effectively concurrent updates to the same resource - if that's possible, then we still need to compare the cache item to what is being put.
There was a problem hiding this comment.
Converted to a comment in the code
shawkins
force-pushed
the
tombstone_removal
branch
from
c0fbec7 to
f621fc9
Compare
| var res = cache.get(resourceID); | ||
| Optional<R> resource = temporaryResourceCache.getResourceFromCache(resourceID); | ||
| if (resource.isPresent()) { | ||
| if (resource.isPresent() && res.filter(r -> r.getMetadata().getResourceVersion() < resource.get().getMetadata().getResourceVersion()).isPresent()) { |
There was a problem hiding this comment.
If events are processed async from the cache update, it's possible for the informer cache to be more up-to-date than the temporary resource cache.
|
|
||
| boolean moveAhead = false; | ||
| String latest = managedInformerEventSource.getLastSyncResourceVersion(resourceId.getNamespace()).orElse(null); | ||
| if (latest != null && latest > newResource.getMetadata().getResourceVersion()) { |
There was a problem hiding this comment.
Is that on zoom at 15:00 CEST?
2 participants
@csviri I misread your working draft at some point and thought you were tracking a single latest revision - that's all I need to remove tombstone tracking. If we are not relying on comparable resource versions, then I think we can simply reuse the informer last synced version to reason about all puts - not just creates.
I've left the latest check as non-compiling as that will line up with changes you have in other prs.