-
Notifications
You must be signed in to change notification settings - Fork 221
Improvements for Quarkus extension #1043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
ce6c12d
ad3e696
179d627
55a903a
1aa554b
1109297
5ef4529
caabe14
0b80e08
36138c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
public interface ResourceTypeAware<R> { | ||
|
||
Class<R> resourceType(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
package io.javaoperatorsdk.operator.processing.dependent; | ||
|
||
import java.lang.reflect.InvocationTargetException; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
@@ -10,6 +9,7 @@ | |
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.fabric8.kubernetes.client.KubernetesClient; | ||
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; | ||
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; | ||
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
@@ -85,25 +85,19 @@ private void initContextIfNeeded(P resource, Context context) { | |
|
||
private DependentResource createAndConfigureFrom(DependentResourceSpec dependentResourceSpec, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could it be move to controller ctor and passed to DependentResourceManager ctor ? It seems weird that dependent are created during prepareEventSources especially when it is not an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I'm following. It makes sense to initialise the That said, I'm open to alternatives. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Looping over a very small list at startup time doesn't worry me that much. Looking at https://github.com/java-operator-sdk/java-operator-sdk/blob/4b3178f6546494faff048b9dcef1e00244586fe3/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconcilerDependentResources.java#L40-L42, the natural choice to instanciate the dependent resources in standalone mode is in the Reconciler ctor. I would expect that the framework is consistent and that the depedent are created with the nature of a dependent resource, they should have been be instantiated in the prepareEventSource too. My POV is that That being said, I checked the code and there is something fishy regarding valid lifetime of DependentResource instance and this will drive when and where it should be instanciated.
As Controller is The last thing is that I don't know how it can ensure sorry for the long story There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't understand this, why would you restart the operator / event source, or in what scenario event source needs to be restartable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a lot to unpack there. I'm not sure we should worry about sources being restartable at this point. The main use case is still an operator that is started once when it is starting up and then stop when it's about to be shutdown. I don't think it makes sense to start/stop the operator multiple times during normal operations. Regarding where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In normal operation, I totally agree. In test suite, this is exactly what does the JUnit Operator extension. It start/stop the same operator instance for each test. I just wanted to warn about that because sample operator are not representative of fully tested operators. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Let's revisit if that becomes an issue. |
||
KubernetesClient client) { | ||
try { | ||
DependentResource dependentResource = | ||
(DependentResource) dependentResourceSpec.getDependentResourceClass() | ||
.getConstructor().newInstance(); | ||
DependentResource dependentResource = | ||
ConfigurationServiceProvider.instance().createFrom(dependentResourceSpec); | ||
|
||
if (dependentResource instanceof KubernetesClientAware) { | ||
((KubernetesClientAware) dependentResource).setKubernetesClient(client); | ||
} | ||
if (dependentResource instanceof KubernetesClientAware) { | ||
((KubernetesClientAware) dependentResource).setKubernetesClient(client); | ||
} | ||
|
||
if (dependentResource instanceof DependentResourceConfigurator) { | ||
final var configurator = (DependentResourceConfigurator) dependentResource; | ||
dependentResourceSpec.getDependentResourceConfiguration() | ||
.ifPresent(configurator::configureWith); | ||
} | ||
return dependentResource; | ||
} catch (InstantiationException | NoSuchMethodException | IllegalAccessException | ||
| InvocationTargetException e) { | ||
throw new IllegalStateException(e); | ||
if (dependentResource instanceof DependentResourceConfigurator) { | ||
final var configurator = (DependentResourceConfigurator) dependentResource; | ||
dependentResourceSpec.getDependentResourceConfiguration() | ||
.ifPresent(configurator::configureWith); | ||
} | ||
return dependentResource; | ||
} | ||
|
||
public List<DependentResource> getDependents() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package io.javaoperatorsdk.operator.processing.dependent.external; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
|
||
public abstract class AbstractPollingDependentResource<R, P extends HasMetadata> | ||
extends AbstractCachingDependentResource<R, P> { | ||
|
||
public static final int DEFAULT_POLLING_PERIOD = 5000; | ||
private long pollingPeriod; | ||
|
||
protected AbstractPollingDependentResource(Class<R> resourceType) { | ||
this(resourceType, DEFAULT_POLLING_PERIOD); | ||
} | ||
|
||
public AbstractPollingDependentResource(Class<R> resourceType, long pollingPeriod) { | ||
super(resourceType); | ||
this.pollingPeriod = pollingPeriod; | ||
} | ||
|
||
public void setPollingPeriod(long pollingPeriod) { | ||
this.pollingPeriod = pollingPeriod; | ||
} | ||
|
||
public long getPollingPeriod() { | ||
return pollingPeriod; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little weird, that a configuration service is responsible for instantiating a Dependent Resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, would be better to add a factory if we want to go that way. Putting it directly on
ConfigurationService
was a shortcut.