eventing refinements mentioned on #2012 #2034

shawkins · 2023-08-25T11:35:55Z

provides two options

to control if the annotation is used (to omit events that come too quickly)
to parse the resource version (to keep the cache up-to-date and omit events if they come too slowly)

Looking for feedback on if these are the right places to introduce the configuration.

csviri · 2023-09-04T07:57:05Z

@shawkins could you pls rebase this PR?

shawkins · 2023-09-05T16:31:30Z

could you pls rebase this PR?

It's up-to-date now.

csviri

Just some minor issues with current code. I'm aware that it is WIP

csviri · 2023-09-06T08:17:30Z

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

+          && ((InformerConfiguration) managedInformerEventSource.getInformerConfiguration())
+              .parseResourceVersions()
+          && Long.compare(Long.parseLong(newResource.getMetadata().getResourceVersion()),
+              Long.parseLong(cachedResource.getMetadata().getResourceVersion())) > 1) {


it should be >0 I guess

Thanks, corrected.

...n/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java

csviri · 2023-09-06T09:27:34Z

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

+    T cachedResource = cache.get(resourceId);
+
+    if (unknownState || (cachedResource != null
+        && !isLaterResourceVersion(resourceId, cachedResource, resource))) {


Kinda makes this code hard to read that the feature flag is checked inside the isLaterResourceVersion, would extract on a level above or rename this function to denote that fact.

Will add a javadoc

csviri · 2023-09-06T09:35:51Z

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

      }
+    } catch (NumberFormatException e) {
+      log.debug(
+          "Could not compare resourceVersions {} and {} for {}",


Shouldn't be this a warning? even if it could pollute cache? (no strong opinion)

It won't pollute the cache, it would exhibit the same behavior as it does now. It can of course be at whatever level you want.

I mean pollute logs :)

shawkins · 2023-09-06T10:22:57Z

Just some minor issues with current code. I'm aware that it is WIP

No problem. Made some changes to address those concerns. What about the methods for configuration hooks - are those in the right places and how far up would that need to be exposed?

csviri · 2023-09-06T12:25:07Z

Just some minor issues with current code. I'm aware that it is WIP

No problem. Made some changes to address those concerns. What about the methods for configuration hooks - are those in the right places and how far up would that need to be exposed?

I think both should be in configuration service, some of the properties we want to configure also on DR level, like if it should use SSA (I will do a separate PR), but not there two IMO

shawkins · 2023-09-11T14:54:03Z

@csviri rebased and further refined the configuration. If this looks good I'll add some tests.

csviri · 2023-09-12T11:36:00Z

@csviri rebased and further refined the configuration. If this looks good I'll add some tests.

sure looks good imo

provides two options - to control if the annotation is used (to omit events that come too quickly) - to parse the resource version (to keep the cache up-to-date and omit events if they come too slowly) Signed-off-by: Steve Hawkins <[email protected]>

metacosm

More generally speaking, shouldn't the event skipping be handled at the ManagedInformerEventSource level instead of in the InformerEventSource? In particular, wouldn't it make sense to handle the resource versions in the temporary cache as well?

metacosm · 2023-09-19T08:36:28Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   *
+   * @since 4.5.0
+   */
+  default boolean previousAnnotationForDependentResources() {


I would rather see a more explicit method name that conveys better the intent, i.e. something like:

Suggested change

default boolean previousAnnotationForDependentResources() {

default boolean processOwnDependentResourceUpdates() {

Since there have been multiple mechanism for this the thinking is to use specific rather than general terminology, so that if it changes again the user can be explicit about which mechanism(s) are in play.

If you strongly prefer the general feature name, that's fine too.

I think this would be too generic.
previousAnnotationForDependentResourcesEventFiltering or somethink would make more sense to me.

metacosm · 2023-09-19T08:37:31Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   *
+   * @since 4.5.0
+   */
+  default boolean parseResourceVersions() {


Similarly, a more explicit method name would be better, imo.

Suggested change

default boolean parseResourceVersions() {

default boolean useResourceVersionsForEventOrdering() {

Let's decide if you even want this before making a name change.

In general I'm not against this. Also would stick with the original name, thus parseResourceVersions since this can have later effect on other parts.

parseResourceVersions doesn't mean anything to me as a user… Parse how? For what purpose? Why would I care? Sure, this can be mitigated by documentation but as we know documentation is not always up to date and I'd rather have explicit method names for user-facing options.

metacosm · 2023-09-19T08:42:16Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -364,4 +364,31 @@ default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
    return Set.of(ConfigMap.class, Secret.class);
  }

+  /**
+   * If an annotation should be used so that the operator sdk can detect events from its own updates


Using an annotation is an implementation detail towards achieving filtering of own updates. If we want to explain that an annotation is used for this purpose, either we need to document in greater details how the annotation is used (so that users can actually use that information) or not mention it at all, imo.

Other than specifying what the annotation name is, there isn't much greater detail that is needed here - the user is not expected to do anything with these annotations.

Yes, I think it would make sense to document the annotation name so that people can look it up in case people notice it in their resources and wonder what that is.

metacosm · 2023-09-19T08:46:21Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   * is typically not needed.
+   * <p>
+   * Disabled by default as Kubernetes does not support, and discourages, this interpretation of
+   * resourceVersions. Enable only if your api server event processing seems to lag the operator


Maybe a more concrete example of when setting this to true would help. As is, it's not obvious when people might want to use this and I would rather not expose this at all if this isn't sufficiently / broadly applicable.

It's basically for feature parity with the mutable cache that is in go client - I'm not aware of user request for the feature, so if you feel strongly about it, then it could be hidden or removed.

@metacosm @csviri what do you think - would it be better to pull out or hide the resourceVersion parsing logic?

shawkins · 2023-09-19T09:58:00Z

More generally speaking, shouldn't the event skipping be handled at the ManagedInformerEventSource level instead of in the InformerEventSource?

Event skipping (filtering or event recording / previous annotation) was already being handled at the InformerEventSource level, this is really just another case to consider.

In particular, wouldn't it make sense to handle the resource versions in the temporary cache as well?

I had considered that, but it didn't initially make sense given the other code structure. Now that does make more sense because parseResourceVersions is passed into the TemporaryCache constructor.

Signed-off-by: Attila Mészáros <[email protected]>

csviri · 2023-09-21T11:36:07Z

...n/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java

@@ -78,14 +80,31 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata>
  private final PrimaryToSecondaryMapper<P> primaryToSecondaryMapper;
  private Map<String, Function<R, List<String>>> indexerBuffer = new HashMap<>();
  private final String id = UUID.randomUUID().toString();
+  private final Set<String> knownResourceVersions;


Please document what are the situations this is solving

Signed-off-by: Attila Mészáros <[email protected]>

csviri · 2023-09-21T11:39:44Z

I like the way it is, just we need to document every change / feature in the PR since it might be hard to follow after time the corner case situatios. But otherwise looks good.

shawkins · 2023-09-21T13:14:19Z

I like the way it is, just we need to document every change / feature in the PR since it might be hard to follow after time the corner case situatios. But otherwise looks good.

@metacosm @csviri made updates based upon the feedback. The config names are now both similar. These are independent of one another, so we're not introducing additional interations. It really is as simple as the annotation will help filter events that come too quickly, and parsing resource versions will help with filtering and cache consistency with events that come too slowly.

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

csviri · 2023-09-21T13:53:33Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   *
+   * @since 4.5.0
+   */
+  default boolean parseResourceVersionsForDependentResourcesEventFilteringAndCaching() {


This kind of a parsing and caching could be done also for the primary resource. For that reason would like to omit that this is specific for the dependent resources, and just maybe add it to the description now.

metacosm · 2023-09-21T14:00:53Z

The tests are also failing consistently so something is fishy there.

shawkins · 2023-09-21T14:10:47Z

The tests are also failing consistently so something is fishy there.

PreviousAnnotationDisabledIT is new. It may need some additional waiting to hit the expectation. I can take a look.

UPDATE: PreviousAnnotationDisabledIT can't have a single hard expectation on the number of executions - if the event arrives quickly an additional reconciliation is triggered. However if the event arrives slowly, then the comparison with the temporary resource cache item will cause the event to be skipped. So I just added a min and max for now.

moving handling of known resource versions, and making the config names more descriptive Signed-off-by: Steve Hawkins <[email protected]>

shawkins · 2023-09-21T15:44:27Z

The remaining test failure is already called out as #2043

* refinements mentioned on #2012 provides two options - to control if the annotation is used (to omit events that come too quickly) - to parse the resource version (to keep the cache up-to-date and omit events if they come too slowly) Signed-off-by: Steve Hawkins <[email protected]>

…ramework#2034) * refinements mentioned on operator-framework#2012 provides two options - to control if the annotation is used (to omit events that come too quickly) - to parse the resource version (to keep the cache up-to-date and omit events if they come too slowly) Signed-off-by: Steve Hawkins <[email protected]>

* refinements mentioned on #2012 provides two options - to control if the annotation is used (to omit events that come too quickly) - to parse the resource version (to keep the cache up-to-date and omit events if they come too slowly) Signed-off-by: Steven Hawkins <[email protected]>

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 Aug 25, 2023

csviri force-pushed the next branch from 5280d26 to 02f843c Compare August 29, 2023 08:25

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2023

csviri force-pushed the next branch 4 times, most recently from 2bdd8f9 to e9c2639 Compare September 4, 2023 08:40

shawkins removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023

shawkins force-pushed the eventing_refinements branch from 8e724e3 to 1ae5efa Compare September 5, 2023 16:42

csviri requested changes Sep 6, 2023

View reviewed changes

csviri reviewed Sep 6, 2023

View reviewed changes

shawkins force-pushed the eventing_refinements branch from 1ae5efa to 535add0 Compare September 6, 2023 10:21

csviri force-pushed the next branch from 39bc7af to a3e2c24 Compare September 11, 2023 08:27

shawkins force-pushed the eventing_refinements branch from 535add0 to 8c412f7 Compare September 11, 2023 12:12

csviri marked this pull request as ready for review September 11, 2023 12:48

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2023

openshift-ci bot requested review from adam-sandor and csviri September 11, 2023 12:48

csviri marked this pull request as draft September 11, 2023 12:48

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 Sep 11, 2023

shawkins force-pushed the eventing_refinements branch 2 times, most recently from 0cc0f5a to 181eb31 Compare September 11, 2023 14:48

csviri force-pushed the next branch from a3e2c24 to 4ec9a19 Compare September 12, 2023 06:51

shawkins force-pushed the eventing_refinements branch from 181eb31 to de90854 Compare September 12, 2023 14:58

metacosm force-pushed the eventing_refinements branch from 03d423d to 1380caa Compare September 19, 2023 08:14

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

metacosm requested changes Sep 19, 2023

View reviewed changes

minor improvements

4c84f9f

Signed-off-by: Attila Mészáros <[email protected]>

csviri reviewed Sep 21, 2023

View reviewed changes

format

05d7951

Signed-off-by: Attila Mészáros <[email protected]>

shawkins requested review from metacosm and csviri September 21, 2023 13:08

shawkins force-pushed the eventing_refinements branch from c5720f0 to 93d9ed2 Compare September 21, 2023 13:19

metacosm reviewed Sep 21, 2023

View reviewed changes

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java Outdated Show resolved Hide resolved

metacosm approved these changes Sep 21, 2023

View reviewed changes

csviri reviewed Sep 21, 2023

View reviewed changes

shawkins force-pushed the eventing_refinements branch from bc613f1 to 905ac9d Compare September 21, 2023 14:24

shawkins requested a review from csviri September 21, 2023 14:28

refinements based upon review feedback

8f00c93

moving handling of known resource versions, and making the config names more descriptive Signed-off-by: Steve Hawkins <[email protected]>

shawkins force-pushed the eventing_refinements branch from 905ac9d to 8f00c93 Compare September 21, 2023 15:34

csviri approved these changes Sep 22, 2023

View reviewed changes

csviri merged commit 42a5358 into operator-framework:next Sep 22, 2023

	default boolean previousAnnotationForDependentResources() {
	default boolean processOwnDependentResourceUpdates() {

	default boolean parseResourceVersions() {
	default boolean useResourceVersionsForEventOrdering() {

eventing refinements mentioned on #2012 #2034

eventing refinements mentioned on #2012 #2034

Conversation

shawkins commented Aug 25, 2023

csviri commented Sep 4, 2023

shawkins commented Sep 5, 2023

csviri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawkins commented Sep 6, 2023

csviri commented Sep 6, 2023

shawkins commented Sep 11, 2023

csviri commented Sep 12, 2023

metacosm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawkins commented Sep 19, 2023

Choose a reason for hiding this comment

csviri commented Sep 21, 2023

shawkins commented Sep 21, 2023

Choose a reason for hiding this comment

metacosm commented Sep 21, 2023

shawkins commented Sep 21, 2023 • edited Loading

shawkins commented Sep 21, 2023

shawkins commented Sep 21, 2023 •

edited

Loading