removing tombstones #2989

shawkins · 2025-10-10T15:48:43Z

@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.

csviri · 2025-10-11T13:56:32Z

...ava/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java


    boolean moveAhead = false;
+    String latest = managedInformerEventSource.getLastSyncResourceVersion(resourceId.getNamespace()).orElse(null);
+    if (latest != null && latest > newResource.getMetadata().getResourceVersion()) {


Is it guaranteed that those events (maybe for different resource names) come in order in watches?

But I guess if we going to compare the resource versions in informers that does not matter.

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.

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!!

( 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 )

Is that on zoom at 15:00 CEST?

shawkins · 2025-10-12T15:38:22Z

...ava/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java

          "Temporarily moving ahead to target version {} for resource id: {}",
          newResource.getMetadata().getResourceVersion(),
          resourceId);
      cache.put(resourceId, newResource);


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.

Converted to a comment in the code

shawkins · 2025-10-12T16:50:23Z

...io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java

+    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()) {


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.

shawkins · 2025-10-13T11:17:47Z

...ava/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java


    boolean moveAhead = false;
+    String latest = managedInformerEventSource.getLastSyncResourceVersion(resourceId.getNamespace()).orElse(null);
+    if (latest != null && latest > newResource.getMetadata().getResourceVersion()) {


Is that on zoom at 15:00 CEST?

csviri · 2025-10-14T13:56:51Z

added branch for 5.3 (as we discuss on community meeting) to target resource version comparison related issues:
#2996

thank you!

shawkins · 2025-10-19T19:05:41Z

@csviri just wanted to double check - what if anything do you want in next? I was thinking that only the annotation removal would go into 5.3.

I have enabled parse versions by default, which does need some refinement - perhaps a better name and we lack a configuration value for on the primary resources. If it is not enabled, the the temporary resource cache won't do anything.

There's no smart exception handling yet. If non comparable versions are seen, then you'll just get an exception - the message could mention disabling the feature.

csviri · 2025-10-20T08:50:11Z

just wanted to double check - what if anything do you want in next? I was thinking that only the annotation removal would go into 5.3.

Although this will work, in almost all cases, since:

There's no smart exception handling yet. If non comparable versions are seen, then you'll just get an exception - the message could mention disabling the feature.

We might want to ship this as a whole in 5.3.
see also: #3011

So we have also more time and refine it. We can then release 5.3 with those feature quite quickly don't have to wait for other feature. What do you think?

csviri · 2025-10-20T08:50:34Z

Do I see that this PR is same as: #3010

Should we close one of those?

csviri

Added just one comment, I lean towards 5.3 regarding this feature, and completely remove the flag for parsing fro that version.
Otherwise LGTM

shawkins · 2025-10-20T10:35:24Z

...ava/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java

-  public synchronized void putAddedResource(T newResource) {
-    putResource(newResource, null);
+            (unknownState
+                    || PrimaryUpdateAndCacheUtils.compareResourceVersions(resource, cached) > 0)


Answering #3010 (comment) here - yes we can switch to >= because we're checking canSkipEvent first. I was starting to think more about the implications of the annotation removal and wondering if we should hold on to the resource that was created until it was obsolete, but we'll discuss that more on the annotation removal pr.

shawkins · 2025-10-20T10:37:02Z

Do I see that this PR is same as: #3010

Should we close one of those?

Closed the other one for now - if enabling parse versions seems like it should be separated, then it will be reopened with all the other changes stripped out.

shawkins · 2025-10-20T10:38:45Z

Added just one comment, I lean towards 5.3 regarding this feature, and completely remove the flag for parsing fro that version. Otherwise LGTM

I don't think it can remove the flag if you want to allow for non-conforming sources. The configuration would also need expanded to the controller itself.

csviri · 2025-10-20T15:17:51Z

Added just one comment, I lean towards 5.3 regarding this feature, and completely remove the flag for parsing fro that version. Otherwise LGTM

I don't think it can remove the flag if you want to allow for non-conforming sources. The configuration would also need expanded to the controller itself.

My idea is that we would detect non-conforming sources dynamically, so if the comparison algorithm throws an error we can flip a flag and don't compare the resources anymore.

csviri · 2025-10-20T19:55:55Z

But for sake of simplicity we could configure it on InformerEventSources for now. (maybe as next PR)

shawkins · 2025-10-21T11:56:24Z

My idea is that we would detect non-conforming sources dynamically, so if the comparison algorithm throws an error we can flip a flag and don't compare the resources anymore.

That could still be two modes of operation:

strict (the default)
lenient

Related to that in this pr there is: https://github.com/operator-framework/java-operator-sdk/pull/2989/files#diff-0b20da91b3c31881f7f78a0ecffefc3811e1916f8e401a047d335fffa8593b71R91-R97

Which is lenient, or it could be done as a breaking change an throw an exception.

csviri · 2025-10-21T14:43:43Z

lenient

Yes, I think lenient is fine, but maybe this would require more discussions. (therefore proposing to move also this pr agains 5.3 branch, and to that version.

Related to that in this pr there is: https://github.com/operator-framework/java-operator-sdk/pull/2989/files#diff-0b20da91b3c31881f7f78a0ecffefc3811e1916f8e401a047d335fffa8593b71R91-R97

I guess the null check is not enough rather a validation of that version, and catching the NonComparableResourceVersionException also at

 public synchronized void putAddedResource(T newResource) {
    putResource(newResource, null);
            (unknownState
                    || PrimaryUpdateAndCacheUtils.compareResourceVersions(resource, cached) >= 0)
                ? null
                : cached);
  }

But the configuration if want to have explicit, should not be on controller level, rather on InformerEventSource+ControllerEventSource level.

Otherwise this looks great!!

shawkins · 2025-10-21T15:01:33Z

Yes, I think lenient is fine, but maybe this would require more discussions. (therefore proposing to move also this pr agains 5.3 branch, and to that version.

Ok, I'll target to the 5.3 branch.

I guess the null check is not enough rather a validation of that version

Null - seems like a simple programatic error, which was already present in the external example
Non-null, but invalid - seems like a non-conforming source, so it seems fine to allow the NonComparableResourceVersionException

and catching the NonComparableResourceVersionException also at

Doesn't it make more sense to start with enabled / disabled behavior, then add a lenient mode later if users want that instead of strictly disabled?

csviri · 2025-10-21T15:25:13Z

Doesn't it make more sense to start with enabled / disabled behavior, then add a lenient mode later if users want that instead of strictly disabled?

I'm fine with that, as far it is per InformerEventSource.

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2025

shawkins force-pushed the tombstone_removal branch from fd5a12c to 91cec06 Compare October 10, 2025 15:49

shawkins mentioned this pull request Oct 10, 2025

Event skipping and temp resource caching based on resourceVersion comparison #2987

Draft

csviri reviewed Oct 11, 2025

View reviewed changes

shawkins force-pushed the tombstone_removal branch from 78af174 to a810ac9 Compare October 12, 2025 15:32

shawkins commented Oct 12, 2025

View reviewed changes

shawkins force-pushed the tombstone_removal branch from c0fbec7 to f621fc9 Compare October 12, 2025 16:50

shawkins commented Oct 13, 2025

View reviewed changes

csviri force-pushed the next branch from 4a02911 to cb87259 Compare October 14, 2025 13:51

csviri force-pushed the next branch from 024d063 to 2902bff Compare October 18, 2025 11:45

shawkins force-pushed the tombstone_removal branch 4 times, most recently from edbe2b6 to 65649f1 Compare October 19, 2025 19:02

csviri reviewed Oct 20, 2025

View reviewed changes

shawkins commented Oct 20, 2025

View reviewed changes

shawkins force-pushed the tombstone_removal branch from 65649f1 to 76b39b1 Compare October 20, 2025 11:31

removing tombstones

b114845

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins force-pushed the tombstone_removal branch from 76b39b1 to b114845 Compare October 21, 2025 17:37

shawkins closed this Oct 21, 2025

shawkins reopened this Oct 21, 2025

shawkins closed this Oct 21, 2025

shawkins mentioned this pull request Oct 21, 2025

removing tombstones #3013

Draft

removing tombstones #2989

removing tombstones #2989

Uh oh!

Conversation

shawkins commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri commented Oct 14, 2025

Uh oh!

shawkins commented Oct 19, 2025

Uh oh!

csviri commented Oct 20, 2025

Uh oh!

csviri commented Oct 20, 2025

Uh oh!

csviri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawkins commented Oct 20, 2025

Uh oh!

shawkins commented Oct 20, 2025

Uh oh!

csviri commented Oct 20, 2025

Uh oh!

csviri commented Oct 20, 2025

Uh oh!

shawkins commented Oct 21, 2025

Uh oh!

csviri commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawkins commented Oct 21, 2025

Uh oh!

csviri commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

csviri Oct 13, 2025 •

edited

Loading

csviri commented Oct 21, 2025 •

edited

Loading

csviri commented Oct 21, 2025 •

edited

Loading