Workflow engine implementation #1153
Conversation
| import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
|
||
| /** | ||
| * Dependents definition: so if B depends on A, the B is dependent of A. |
There was a problem hiding this comment.
I think this is confusing since it's the reverse of what we use for DependentResource: the primary resource depends on the dependents…
There was a problem hiding this comment.
Yep, I think the naming needs to be improved here in general. So this is a directed graph at the end, if reverse direction we could use simply parents. Not sure if dependents is actually bad, but yes can be confused with dependent resource from other context, maybe just use child?
csviri
force-pushed
the
next
branch
2 times, most recently
from
60771e8 to
3666014
Compare
csviri
force-pushed
the
workflow
branch
3 times, most recently
from
011ad48 to
3972845
Compare
There was a problem hiding this comment.
I think this is a good start we can merge.
Some comments:
- I think the names being used are too confusing… I especially got confused by the dependents field in Workflow initially (and actually got rid of it in part because of this ^_^)
- We should have the code fully reactive and only wait for all resources to be processed at the top level
- Should we use a DAG library? We could probably find one that would handle our use case without needing to maintain our own (especially if we factor in other things like cycle detection and reduction of the graph)
| public WebPageDependentsWorkflowReconciler(KubernetesClient kubernetesClient) { | ||
| initDependentResources(kubernetesClient); | ||
| workflow = new WorkflowBuilder<WebPage>() | ||
| .addDependent(configMapDR).build() |
There was a problem hiding this comment.
you mean the addDependent ? could be addDependentResource(), or?
| return erroredDependents; | ||
| } | ||
|
|
||
| public WorkflowExecutionResult setErroredDependents( |
There was a problem hiding this comment.
If we're not chaining calls, we shouldn't return this, imo…
| .getTopLevelDependentResources()) { | ||
| handleReconcile(dependentResourceNode); | ||
| } | ||
| while (true) { |
There was a problem hiding this comment.
Wouldn't it be better to have the whole implementation reactive and block at a higher level instead of idling here?
There was a problem hiding this comment.
Would be. First I tried with reactive, but this has quite dynamic nature, while with reactive (CompletableFuture etc) is meant to for static workflows. Pretty sure it would be possible somehow just probably not skilled enough in this.
|
Kudos, SonarCloud Quality Gate passed! |
Well we can take a look, but there are parts when the dag is actually traversed back and forth, for example on reconcile delete is made in reverse order on a sub-tree. |
No description provided.