-
Notifications
You must be signed in to change notification settings - Fork 217
refactor: rename "bulk" to more accurate "dynamically created" #1538
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 all commits
edb17f2
bad4d3b
6f59d12
94b056a
ca390ef
2ab160e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,29 +13,29 @@ | |
import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result; | ||
import io.javaoperatorsdk.operator.processing.event.ResourceID; | ||
|
||
class BulkDependentResourceReconciler<R, P extends HasMetadata> | ||
class DynamicallyCreatedDependentResourceReconciler<R, P extends HasMetadata> | ||
implements DependentResourceReconciler<R, P> { | ||
|
||
private final BulkDependentResource<R, P> bulkDependentResource; | ||
private final DynamicallyCreatedDependentResource<R, P> delegate; | ||
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. As mentioned don't agree with term 'Dynamic' it's too generic notion. Why Bulk nicely identifies this feature. 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. Bulk doesn't fit either, imo. It kinda worked when the solution was indexed-based but now it's more about dynamically (as opposed to statically known at build time) defined dependents. 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. But the whole feature / funtionality is about to create resources in bulk. That is the only purpose, the only thing that changed is that the key is not an integer but a string. 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. Bulk implies a large number and that's not really what the feature is about. This is about creating some resources that are determined at runtime, as opposed to the other dependents which are known statically at build-time. I agree that "dynamically created" isn't too appropriate but I don't think that "bulk" is, either… |
||
|
||
BulkDependentResourceReconciler(BulkDependentResource<R, P> bulkDependentResource) { | ||
this.bulkDependentResource = bulkDependentResource; | ||
DynamicallyCreatedDependentResourceReconciler( | ||
DynamicallyCreatedDependentResource<R, P> delegate) { | ||
this.delegate = delegate; | ||
} | ||
|
||
@Override | ||
public ReconcileResult<R> reconcile(P primary, Context<P> context) { | ||
final var desiredResources = bulkDependentResource.desiredResources(primary, context); | ||
Map<String, R> actualResources = bulkDependentResource.getSecondaryResources(primary, context); | ||
final var desiredResources = delegate.desiredResources(primary, context); | ||
Map<String, R> actualResources = delegate.getSecondaryResources(primary, context); | ||
|
||
// remove existing resources that are not needed anymore according to the primary state | ||
deleteBulkResourcesIfRequired(desiredResources.keySet(), actualResources, primary, context); | ||
deleteUnexpectedDynamicResources(desiredResources.keySet(), actualResources, primary, context); | ||
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. It's not really unexpected. It's natural to have those. Maybe "surplus" ? but not sure if this word used in this context. 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. What about 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. sounds great! 👍 |
||
|
||
final List<ReconcileResult<R>> results = new ArrayList<>(desiredResources.size()); | ||
final var updatable = bulkDependentResource instanceof Updater; | ||
final var updatable = delegate instanceof Updater; | ||
desiredResources.forEach((key, value) -> { | ||
final var instance = | ||
updatable ? new UpdatableBulkDependentResourceInstance<>(bulkDependentResource, value) | ||
: new BulkDependentResourceInstance<>(bulkDependentResource, value); | ||
final var instance = updatable ? new UpdatableDynamicDependentInstance<>(delegate, value) | ||
: new DynamicDependentInstance<>(delegate, value); | ||
results.add(instance.reconcile(primary, actualResources.get(key), context)); | ||
}); | ||
|
||
|
@@ -44,41 +44,42 @@ public ReconcileResult<R> reconcile(P primary, Context<P> context) { | |
|
||
@Override | ||
public void delete(P primary, Context<P> context) { | ||
var actualResources = bulkDependentResource.getSecondaryResources(primary, context); | ||
deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context); | ||
var actualResources = | ||
delegate.getSecondaryResources(primary, context); | ||
deleteUnexpectedDynamicResources(Collections.emptySet(), actualResources, primary, context); | ||
} | ||
|
||
private void deleteBulkResourcesIfRequired(Set<String> expectedKeys, | ||
private void deleteUnexpectedDynamicResources(Set<String> expectedKeys, | ||
Map<String, R> actualResources, P primary, Context<P> context) { | ||
actualResources.forEach((key, value) -> { | ||
if (!expectedKeys.contains(key)) { | ||
bulkDependentResource.deleteBulkResource(primary, value, key, context); | ||
delegate.deleteTargetResource(primary, value, context); | ||
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. +1 |
||
} | ||
}); | ||
} | ||
|
||
/** | ||
* Exposes a dynamically-created instance of the bulk dependent resource precursor as an | ||
* AbstractDependentResource so that we can reuse its reconciliation logic. | ||
* Exposes a dynamically-created instance of the dynamically-created dependent resource precursor | ||
* as an AbstractDependentResource so that we can reuse its reconciliation logic. | ||
* | ||
* @param <R> | ||
* @param <P> | ||
*/ | ||
private static class BulkDependentResourceInstance<R, P extends HasMetadata> | ||
private static class DynamicDependentInstance<R, P extends HasMetadata> | ||
extends AbstractDependentResource<R, P> | ||
implements Creator<R, P>, Deleter<P> { | ||
private final BulkDependentResource<R, P> bulkDependentResource; | ||
private final DynamicallyCreatedDependentResource<R, P> delegate; | ||
private final R desired; | ||
|
||
private BulkDependentResourceInstance(BulkDependentResource<R, P> bulkDependentResource, | ||
private DynamicDependentInstance(DynamicallyCreatedDependentResource<R, P> delegate, | ||
R desired) { | ||
this.bulkDependentResource = bulkDependentResource; | ||
this.delegate = delegate; | ||
this.desired = desired; | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
private AbstractDependentResource<R, P> asAbstractDependentResource() { | ||
return (AbstractDependentResource<R, P>) bulkDependentResource; | ||
return (AbstractDependentResource<R, P>) delegate; | ||
} | ||
|
||
@Override | ||
|
@@ -88,12 +89,13 @@ protected R desired(P primary, Context<P> context) { | |
|
||
@SuppressWarnings("unchecked") | ||
public R update(R actual, R desired, P primary, Context<P> context) { | ||
return ((Updater<R, P>) bulkDependentResource).update(actual, desired, primary, context); | ||
return ((Updater<R, P>) delegate).update(actual, desired, primary, | ||
context); | ||
} | ||
|
||
@Override | ||
public Result<R> match(R resource, P primary, Context<P> context) { | ||
return bulkDependentResource.match(resource, desired, primary, context); | ||
return delegate.match(resource, desired, primary, context); | ||
} | ||
|
||
@Override | ||
|
@@ -113,7 +115,7 @@ public Class<R> resourceType() { | |
|
||
@Override | ||
public R create(R desired, P primary, Context<P> context) { | ||
return bulkDependentResource.create(desired, primary, context); | ||
return delegate.create(desired, primary, context); | ||
} | ||
} | ||
|
||
|
@@ -123,13 +125,12 @@ public R create(R desired, P primary, Context<P> context) { | |
* @param <R> | ||
* @param <P> | ||
*/ | ||
private static class UpdatableBulkDependentResourceInstance<R, P extends HasMetadata> | ||
extends BulkDependentResourceInstance<R, P> implements Updater<R, P> { | ||
private static class UpdatableDynamicDependentInstance<R, P extends HasMetadata> | ||
extends DynamicDependentInstance<R, P> implements Updater<R, P> { | ||
|
||
private UpdatableBulkDependentResourceInstance( | ||
BulkDependentResource<R, P> bulkDependentResource, | ||
private UpdatableDynamicDependentInstance(DynamicallyCreatedDependentResource<R, P> delegate, | ||
R desired) { | ||
super(bulkDependentResource, desired); | ||
super(delegate, desired); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package io.javaoperatorsdk.operator.processing.dependent; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
/** | ||
* Helper for the dynamically-created Dependent Resources to make it more explicit that such | ||
* dependents only to implement | ||
* {@link DynamicallyCreatedDependentResource#match(Object, Object, HasMetadata, Context)} | ||
* | ||
* @param <R> secondary resource type | ||
* @param <P> primary resource type | ||
*/ | ||
public interface DynamicallyCreatedUpdater<R, P extends HasMetadata> extends Updater<R, P> { | ||
|
||
default Matcher.Result<R> match(R actualResource, P primary, Context<P> context) { | ||
if (!(this instanceof DynamicallyCreatedDependentResource)) { | ||
throw new IllegalStateException(DynamicallyCreatedUpdater.class.getSimpleName() | ||
+ " interface should only be implemented by " | ||
+ DynamicallyCreatedDependentResource.class.getSimpleName() + " implementations"); | ||
} | ||
throw new IllegalStateException("This method should not be called from a " | ||
+ DynamicallyCreatedDependentResource.class.getSimpleName() + " implementation"); | ||
} | ||
} |
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.
+1 for rename
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.
But not sure we want to remove the key. I'm more to more complete APIs, so that is an information that some might miss if providing a custom implementation. So this way it's more future proof, also integral part.
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.
OK, was debating this last point myself, actually…