feat: add a DependentResource implementation that's also an EventSource #1094
Conversation
|
Generally speaking, I think https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/rds-cloudwatch-events.sample.html In this case the event would trigger the reconciliation, but the dependent resource would not have an event source, especially not a caching event source. (See also: https://aws.amazon.com/premiumsupport/knowledge-center/create-rds-event-subscription/ ) see also the |
| private final Class<R> resourceType; | ||
|
|
||
| protected AbstractCachingDependentResource(Class<R> resourceType) { | ||
| this.resourceType = resourceType; | ||
| } | ||
|
|
||
| public Optional<R> fetchResource(P primaryResource) { | ||
| return eventSource.getAssociated(primaryResource); |
There was a problem hiding this comment.
I know that you did not changed this, but probably the featch resource should be abstract. The idea is that it does not read from the cache.
There was a problem hiding this comment.
Can we do this in a separate PR?
The benefit of this change is multi-fold: - simplifies AbstractDependentResource which doesn't have to handle cases it should not be concerned about - allows to share more code with AbstractSimpleDependentResource - enforces that the same name is used for the DependentResource and EventSource even in the standalone mode when using one of the provided implementations
metacosm
force-pushed
the
dependent-event-source-2
branch
from
2fe5d60 to
fcace92
Compare
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceOwner.java
Show resolved
Hide resolved
| * newly created resource | ||
| * @param created the newly created resource | ||
| */ | ||
| protected abstract void processPostCreate(ResourceID primaryResourceId, R created); |
There was a problem hiding this comment.
would probably rename to postProcessCreate.
There was a problem hiding this comment.
I don't see it. This method, and the other similar one, is to process the given resource post-create, so I think it makes more sense the way it currently is. However, since it seems confusing, maybe something more event-oriented would work better like onCreated?
...rc/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java
Outdated
Show resolved
Hide resolved
| @@ -37,56 +37,50 @@ | |||
| private static final Logger log = | |||
| LoggerFactory.getLogger(WebPageStandaloneDependentsReconciler.class); | |||
|
|
|||
| private KubernetesDependentResource<ConfigMap, WebPage> configMapDR; | |||
There was a problem hiding this comment.
I think this is an unnecessary simplification I actually plan to extend this sample with some if or other workflow enahncements, that is not possible with managed dependent resources.
Basically that is the point that we can hold a simple reference to the dependent resource, that can be easily accessed. Putting it into a map and accessing it via String, is just unnecessary complicaiton.
See also other PR pl with defaul naming.
There was a problem hiding this comment.
Sure but the current example only shows code duplication which isn't really something we want people to replicate so I'd rather leave it as it is for this PR and you can make the modifications you want on it in a subsequent PR.
There was a problem hiding this comment.
you mean code duplicaiton here:
private void createDependentResources(KubernetesClient client) {
this.configMapDR = new ConfigMapDependentResource();
this.configMapDR.setKubernetesClient(client);
configMapDR.configureWith(new KubernetesDependentResourceConfig()
.setLabelSelector(DEPENDENT_RESOURCE_LABEL_SELECTOR));
this.deploymentDR = new DeploymentDependentResource();
deploymentDR.setKubernetesClient(client);
deploymentDR.configureWith(new KubernetesDependentResourceConfig()
.setLabelSelector(DEPENDENT_RESOURCE_LABEL_SELECTOR));
this.serviceDR = new ServiceDependentResource();
serviceDR.setKubernetesClient(client);
serviceDR.configureWith(new KubernetesDependentResourceConfig()
.setLabelSelector(DEPENDENT_RESOURCE_LABEL_SELECTOR));
}
?
There was a problem hiding this comment.
And calling reconcile on each dependent and create the dependents each exactly the same way… Basically, as it stands, this sample isn't really interesting (apart from showing that people should probably use managed dependents for this kind of use case 😄) so I'd just leave it as it is here and then we can make more changes to make it more interesting in other PRs.
There was a problem hiding this comment.
Yes, right, will come up with something to improve it.
Btw otherwise look good to me. Happy to merge! Pls do. ( Just don't know how to approve from mobile app :) )
|
Kudos, SonarCloud Quality Gate passed! |
The benefit of this change is multi-fold:
cases it should not be concerned about
EventSource even in the standalone mode when using one of the provided
implementations