Persistent state dependent resources #1543

csviri · 2022-10-13T11:58:36Z

No description provided.

Co-authored-by: Chris Laprun <metacosm@gmail.com>

* feat: decouple event source from cache + list discriminator (#1378) * feat: bulk dependent resources (#1448) * feat: optional eventsource on dependent resources (#1479) * refactor: simplify handling of reused event sources (#1518) Co-authored-by: Chris Laprun <metacosm@gmail.com> * refactor: isolate index handling to BulkDependentResource interface (#1517) * feat: key based bulk resource creation (#1521) * improvement: bulk dependent resource api * merge Co-authored-by: Chris Laprun <metacosm@gmail.com> Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

* refactor: deleteBulkResource -> deleteTargetResource * refactor: remove now unneeded class * refactor: deleteBulkResourcesIfRequired -> deleteExtraResources * fix: javadoc

csviri · 2022-10-19T11:53:38Z

closes #1221

...rc/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java

metacosm · 2022-10-20T16:33:23Z

...java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java

+public abstract class AbstractExternalDependentResource<R, P extends HasMetadata, T extends ResourceEventSource<R, P>>
+    extends AbstractEventSourceHolderDependentResource<R, P, T> {
+
+  private final boolean isDependentResourceWithExplicitState =


Would someone use this class and not implement DependentResourceWithExplicitState? If yes, why?

The thing is that the "batteries included" externals should support easily external resources with and without the state. So it should not implement DependentResourceWithExplicitState by default.
But you got a point, that if some just want to imlpement an external resource event source without state, might just extend AbstractEventSourceHolderDependentResource.

Maybe we should rename it to ExternalWithStateSupportDependentResource ?

I'm not sure renaming the class will fix the problem… Why would someone extend AbstractExternalDependentResource and not implement DependentResourceWithExplicitState? Could you detail a use case where someone would extend this class and not want an explicit external state with it?

yep, so we have already samples for that (like the mysql sample):
If somebody want a PollingDependentResource, that does not need an external state.

Note that not every external resource needs an explicit state, for example if it can be addressed only by the information in the custom resource. However some might need one, like if a github PR is managed. Therefore the current implementation allows to a dependent resource to extend PollingDependentResource (and others) and to decide if it needs an external state or not implementing the related DependentResourceWithExplicitState.

I thing that we might address in a separate PR and could have an impact on this is this issue:
#1511
(but not necessarily)

However I recognize that Polling and PerResourcePolling DRs are quite a thin layer, that basically just creates an instance of a EventSource on top of a standard functionality. (althoug might be extended later for example with an improved configuration). So the structure might change in the future if we for example make it less explic? "Thus just an external event source that provides an event source" abstraction.

It seems, though, that (most of) the functionality that this class provides is only available if you implement DependentResourceWithExplicitState so I'm wondering about the benefit of having this class plus a separate interface when most (if not all of the behavior) is specific to that one use case.

Yep, but don't see how we would cover otherwise just optionally having this external state functionality for Polling EventSources.

Co-authored-by: Chris Laprun <metacosm@gmail.com>

…1517)

* feat: decouple event source from cache + list discriminator (#1378) * feat: bulk dependent resources (#1448) * feat: optional eventsource on dependent resources (#1479) * refactor: simplify handling of reused event sources (#1518) Co-authored-by: Chris Laprun <metacosm@gmail.com> * refactor: isolate index handling to BulkDependentResource interface (#1517) * feat: key based bulk resource creation (#1521) * improvement: bulk dependent resource api * merge Co-authored-by: Chris Laprun <metacosm@gmail.com> Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

* refactor: deleteBulkResource -> deleteTargetResource * refactor: remove now unneeded class * refactor: deleteBulkResourcesIfRequired -> deleteExtraResources * fix: javadoc

# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/SingleDependentResourceReconciler.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractPollingDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

metacosm · 2022-10-25T11:52:43Z

...k/operator/sample/externalstate/ExternalWithStateDependentResourceDependentResourceWith.java

+
+import static io.javaoperatorsdk.operator.sample.externalstate.ExternalStateDependentReconciler.ID_KEY;
+
+public class ExternalWithStateDependentResourceDependentResourceWith extends


Can we rename this class to something better?

metacosm · 2022-10-25T11:59:47Z

...java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java

+public abstract class AbstractExternalDependentResource<R, P extends HasMetadata, T extends ResourceEventSource<R, P>>
+    extends AbstractEventSourceHolderDependentResource<R, P, T> {
+
+  private final boolean isDependentResourceWithExplicitState =


It seems, though, that (most of) the functionality that this class provides is only available if you implement DependentResourceWithExplicitState so I'm wondering about the benefit of having this class plus a separate interface when most (if not all of the behavior) is specific to that one use case.

metacosm · 2022-10-25T12:01:05Z

...javaoperatorsdk/operator/processing/dependent/external/AbstractPollingDependentResource.java


 @Ignore
 public abstract class AbstractPollingDependentResource<R, P extends HasMetadata>
-    extends AbstractCachingDependentResource<R, P> implements CacheKeyMapper<R> {
+    extends AbstractExternalDependentResource<R, P, ExternalResourceCachingEventSource<R, P>>
+    implements CacheKeyMapper<R> {



Is a polling dependent resource necessarily external?

Yes, we don't poll kubernetes API.

metacosm · 2022-10-25T12:01:48Z

...io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

@@ -1,16 +1,13 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

 import java.util.HashMap;
-import java.util.Optional;
 import java.util.Set;

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;


Should we throw an exception if secondary is null?

this is an outdated place, could you pls point me again where do you mean it?

# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/SingleDependentResourceReconciler.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractPollingDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

sonarqubecloud · 2022-10-25T15:03:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
28 Code Smells

32.1% Coverage
0.0% Duplication

csviri and others added 9 commits October 6, 2022 11:04

feat: decouple event source from cache + list discriminator (#1378)

3ff673f

feat: bulk dependent resources (#1448)

dcd3718

feat: optional eventsource on dependent resources (#1479)

fa7fa28

refactor: simplify handling of reused event sources (#1518)

d8da98b

Co-authored-by: Chris Laprun <metacosm@gmail.com>

refactor: isolate index handling to BulkDependentResource interface (#…

bf7a79d

…1517)

feat: key based bulk resource creation (#1521)

3cd36d1

fix: fixes after rebase

0092f0a

refactor: isolate bulk dependent resource handling more (#1530)

92ec34e

csviri self-assigned this Oct 13, 2022

csviri force-pushed the persistent-state-dependent-resources branch 2 times, most recently from d483f7b to 74fd92f Compare October 13, 2022 14:36

csviri and others added 2 commits October 17, 2022 18:55

feat: external resource with generated id support (#1527)

bbf4d80

refactor: deleteBulkResource -> deleteTargetResource (#1544)

5a0cb3a

* refactor: deleteBulkResource -> deleteTargetResource * refactor: remove now unneeded class * refactor: deleteBulkResourcesIfRequired -> deleteExtraResources * fix: javadoc

csviri mentioned this pull request Oct 18, 2022

[wip] feat: external resource with provided ID handling #1502

Closed

csviri requested a review from metacosm October 18, 2022 08:09

csviri added 14 commits October 18, 2022 16:40

feat: external state management for dependent resources

a456441

wip

e1a89c7

wip

0bc7f92

wip

50ebb9b

wip

beacbb6

wip

4e45517

Integration test

02c6b4a

test fix

aff1fcc

bulk + external state

a8d9c9f

IT refactor

7b9e6c7

format

c270875

fix

80d9ea0

docs

73d84d0

refactors

b1e6dcb

metacosm reviewed Oct 20, 2022

View reviewed changes

...rc/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java Outdated Show resolved Hide resolved

fix: exception wording

6ad757d

metacosm reviewed Oct 20, 2022

View reviewed changes

csviri and others added 12 commits October 21, 2022 11:05

format

4c5a52d

feat: decouple event source from cache + list discriminator (#1378)

9fa7236

feat: bulk dependent resources (#1448)

37ca487

feat: optional eventsource on dependent resources (#1479)

db4951d

refactor: simplify handling of reused event sources (#1518)

a80b367

Co-authored-by: Chris Laprun <metacosm@gmail.com>

refactor: isolate index handling to BulkDependentResource interface (#…

ce9fc7d

…1517)

feat: key based bulk resource creation (#1521)

c61c6bc

fix: fixes after rebase

1757b29

refactor: isolate bulk dependent resource handling more (#1530)

0a84412

feat: external resource with generated id support (#1527)

ba69350

refactor: deleteBulkResource -> deleteTargetResource (#1544)

31502ce

* refactor: deleteBulkResource -> deleteTargetResource * refactor: remove now unneeded class * refactor: deleteBulkResourcesIfRequired -> deleteExtraResources * fix: javadoc

csviri force-pushed the next branch from 15e30f1 to 31502ce Compare October 21, 2022 12:55

csviri added 2 commits October 24, 2022 10:21

merge from next

e37736e

csviri force-pushed the next branch from 31502ce to 72158d9 Compare October 25, 2022 11:09

metacosm requested changes Oct 25, 2022

View reviewed changes

csviri added 2 commits October 25, 2022 16:55

rename

3dbdde6

csviri requested a review from metacosm October 26, 2022 09:47

metacosm approved these changes Oct 26, 2022

View reviewed changes

csviri merged commit f9f7b52 into next Oct 26, 2022

csviri deleted the persistent-state-dependent-resources branch October 26, 2022 14:19

csviri added a commit that referenced this pull request Oct 28, 2022

Persistent state dependent resources (#1543)

1624f1b

csviri added a commit that referenced this pull request Oct 31, 2022

Persistent state dependent resources (#1543)

6592591


		import static io.javaoperatorsdk.operator.sample.externalstate.ExternalStateDependentReconciler.ID_KEY;

		public class ExternalWithStateDependentResourceDependentResourceWith extends

Persistent state dependent resources #1543

Persistent state dependent resources #1543

Uh oh!

Conversation

csviri commented Oct 13, 2022

Uh oh!

csviri commented Oct 19, 2022

Uh oh!

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 Oct 25, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 25, 2022

Uh oh!

Uh oh!

csviri Oct 25, 2022 •

edited

Loading