hansva commented on PR #8560:
URL: https://github.com/apache/hop/pull/8560#issuecomment-5814173097
### Must fix
1. **Beam pipelines can't be run again** (`HopGuiPipelineGraph.java:6083`
and `:6290`)
The start/debug check changed from `!isRunning()` to `pipeline == null ||
pipeline.isFinished()`. `BeamPipelineEngine` only calls `setFinished(true)`
when the run FAILED. The DONE, STOPPED and CANCELLED cases set `running =
false` and the status, but never `finished`. After any Beam run that completes
normally, pressing Run shows "This pipeline is already running" every time
until the tab is closed.
2. **Failed preparation probably blocks retries too**
When preparation fails, `Pipeline.flagPreparationFailure` calls
`stopAll()`, which leaves the engine stopped. Please check whether `stopAll()`
ever sets `finished`. If it doesn't, then after a refused transform or an
unreachable remote server, Run is enabled but every retry says "already
running". The session guard already handles stale listeners, so the stricter
check adds nothing. Going back to `!isRunning()` fixes both 1 and 2.
3. **The GUI can freeze: lock-order inversion**
(`HopGuiPipelineGraph.java:6533`)
- The finished listener takes the session lock and then calls
`checkPipelineEnded()`. After a preview/debug run with zero breakpoint hits,
that calls `showLastPreviewResults()`, which is `synchronized` on the graph.
- `start()` and `debug()` are `synchronized` on the graph and take the
session lock through `setDisplayedPipeline` → `adopt`.
If a preview run finishes just as the user presses Run, each thread waits
for the other, permanently. Before this change there was only one lock, so this
couldn't happen. Suggested fix: take the session lock only to check that the
run is still current, then call `checkPipelineEnded()` and the rest after
releasing it.
### Should fix
4. **The Kafka fix does the same thing twice**
(`KafkaConsumerInput.java:214`)
The change removes the only call to `buildExecutionSummary()`, and that
method is the only place `clearingMetricsPerIteration` is read. So
`setClearingMetricsPerIteration(false)` now does nothing, and the comment
"Mapping and Beam still clear per iteration" is wrong because nothing else
calls it. Pick one approach:
- Keep the call and the flag. The sub-pipeline keeps its per-batch
summary log lines.
- Remove the call and delete the now-unused flag and method.
5. **Listener code runs while holding the session lock**
(`HopGuiPipelineGraph.java:6200`)
`pipelineFinished` runs extension points and may show an `ErrorDialog`.
The GUI thread takes the same lock on every redraw tick. The class Javadoc says
the runnable "must not wait on the GUI thread", but plugin code in extension
points can't be held to that.
6. **Old redraw timers are never cancelled**
(`HopGuiPipelineGraph.java:6659`, `HopGuiWorkflowGraph.java:6049`)
A timer whose engine is no longer current skips its work but keeps
running. The old engine's finished listener, which used to cancel it, is now
ignored, so one Timer thread can be leaked per occurrence. Calling `cancel()`
when `isCurrent()` is false fixes it.
### Smaller points
- **Workflow `start()` has a timer race** (`HopGuiWorkflowGraph.java:5467`).
The thread and the redraw timer start before the finished and stopped listeners
are added. A trivial workflow that finishes in that window leaves the timer
running. This problem already existed, but this change is where it gets fixed.
- **The engine and generation are read in two steps.** Having `adopt()`
return both together as one snapshot would close that gap and remove the three
pass-through methods on the graph (`currentExecutionGeneration`,
`isCurrentExecution`, `runIfCurrentExecution`).
- **Duplicated code.** The timer-install logic is copied between the
pipeline and workflow graphs, and the finished-listener lambda appears in both
`startThreads` and `attachToRunningPipeline`. A shared helper would keep future
fixes (such as 6) in one place.
- **The Kafka first-row-date check runs for every record**
(`KafkaConsumerInput.java:361`). Moving it above the loop runs it once per
batch.
### Suggested way forward
Keep the Kafka part once 4 is tidied up. For the GUI part, go back to
`!isRunning()` to fix 1 and 2, and move the listener's work out from under the
session lock to fix 3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
