AP-25344: CombinedExecutor: option to remove failed segments from com… #23
Conversation
…bined workflow ... and improved handling of failing segments executed with combined executor. AP-25344 (Properly handle failing tools in combined tools workflow)
There was a problem hiding this comment.
Pull request overview
This PR improves error handling in the CombinedExecutor by adding an option to remove failed workflow segments from the combined workflow, and ensures node messages are always collected when execution fails.
- Added
removeFailedSegmentsconfiguration option to CombinedExecutor.Builder - Refactored execution result creation to handle success and failure cases separately
- Enhanced WorkflowToolCell to properly handle null results when segments fail
- Updated documentation to clarify message collection behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| WorkflowSegmentExecutor.java | Updated javadoc to clarify that messages are always collected on execution failure |
| CombinedExecutor.java | Added removeFailedSegments option and split result creation into success/failure paths with proper cleanup |
| WorkflowToolCell.java | Added null safety checks for failed execution results and extracted view node ID logic |
| WorkflowToolCellTest.java | Added comprehensive test for failing tool execution scenarios with and without segment removal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
org.knime.core/src/eclipse/org/knime/core/node/workflow/capture/CombinedExecutor.java
Show resolved
Hide resolved
| metadataBuilder.withInPort(pm.name(), pm.description()); | ||
| } | ||
| var outPortMetadata = new Port[component.getNrOutPorts() - 1]; | ||
| var outPortMetadata = new Port[outputs == null ? 0 : component.getNrOutPorts() - 1]; |
There was a problem hiding this comment.
The ternary expression is computing array size but component could be null (as checked on line 585). Consider using a guard condition to return early if outputs is null, as component.getNrOutPorts() would throw NullPointerException if component is null.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
the component can never be null here (see null check and early return on top)
| metadataBuilder.withInPort(pm.name(), pm.description()); | ||
| } | ||
| var outPortMetadata = new Port[component.getNrOutPorts() - 1]; | ||
| var outPortMetadata = new Port[outputs == null ? 0 : component.getNrOutPorts() - 1]; |
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
hornm-knime
deleted the
todo/AP-25344-properly-handle-failing-tools-i
branch
…bined workflow
... and improved handling of failing segments executed with combined executor.
AP-25344 (Properly handle failing tools in combined tools workflow)