bamaer commented on PR #8625:
URL: https://github.com/apache/hop/pull/8625#issuecomment-5854144827
Bounded cache, log cap, `retrieveIds()` statement fix, `insertRow()`
try/finally, and the heavy-document split all look right. Two blockers.
### Blockers
**1. `appendLoggingDelta()` duplicates logging text for every engine still
passing `-1`.**
`ExecutionStateBuilder.fromExecutor()` always sets `lastLogLineNr`
(`ExecutionStateBuilder.java:100`, `withLastLogLineNr(lastNrInLogStore)`), so
the `!= null` test in `appendLoggingDelta` is always true and the "full
snapshot replaces" branch is unreachable in production. Callers still passing
`-1` return the whole buffer, which now gets appended:
- `LocalWorkflowEngine:533` (final state, after delta ticks) → workflow log
stored twice.
- `BeamPipelineEngine:1187`/`:1213`, `BeamDataFlowPipelineEngine:67`,
`SparkPipelineEngine:635`/`:793`, `LoadBalancingPipelineEngine:218`/`:270`,
`LoadBalancingWorkflowEngine:192`/`:245` → full buffer appended every tick
until it saturates `MAX_CACHED_LOGGING_TEXT_CHARS`.
```
appendLoggingDelta(null, state("line1\n", 1)) -> "line1\n"
appendLoggingDelta(prev, state("line1\nline2\n", 2)) ->
"line1\nline1\nline2\n"
```
Fix: signal "this is a delta" from the caller — leave `lastLogLineNr` null
when `-1` was requested, or add an explicit flag — and move the remaining
engines to incremental line numbers.
**2. `LocalPipelineEngine.stopAll()` closes the location before the pipeline
has a terminal state.**
`Pipeline.stopAll()` only sets `stopped`; `finished` and `executionEndDate`
are set later (`Pipeline.java:1365-1374`). The override writes the state as of
stop time, nulls `executionInfoLocation` and closes, so `pipelineCompleted()` →
`stopTransformExecutionInfoTimer()` returns immediately and the end date,
duration, final status, final row counts and shutdown log lines are never
stored.
Reachable from `HopGuiPipelineGraph:5886` (Stop button),
`StopPipelineServlet:125`, and `RunThread:93` → `BaseTransform.stopAll()` →
`pipeline.stopAll()` — i.e. any transform that throws.
Fix: guard on `getPipelineMeta().getPipelineType() ==
PipelineType.SingleThreaded`, or close from the finished path with a separate
guard for single-threaded children.
### Should fix
**3. DB connection leaks when the final flush fails.** `close()` skips
`discardDatabase()` while entries are dirty, but the retry it defers to never
happens: `stopTransformExecutionInfoTimer()` is the only caller, calls
`close()` once and nulls the location, and `cacheClosed = true` has already
stopped the cache timer. Drop the connection unconditionally (logging the
loss), or give the retry a trigger.
**4. `ensureStateJsonColumn()` runs `ALTER TABLE ... ADD state_json` on
initialize.** This location otherwise has the user run `buildDdl` output
themselves. Put the column in that GUI action and rely on the existing
`stateJsonColumnAvailable` runtime fallback instead of migrating a production
schema on the first run.
**5. `BaseCachingExecutionInfoLocationTest` is order-dependent.** Run alone,
`enforceMaxCacheSize`'s `LogChannel.GENERAL.logError()` throws out of its own
catch block:
```
evictionKeepsADirtyEntryWhenPersistFails
expected: <HopException> but was: <HopRuntimeException>
Caused by: HopRuntimeException: Central Log Store is not initialized!!!
at BaseCachingExecutionInfoLocation.enforceMaxCacheSize(...:282)
```
Add `HopLogStore.init()` in `@BeforeAll`, as
`LocalPipelineEngineExecutionIdTest` does.
### Verified
Built `a982ed1` in a throwaway worktree:
`CachingDatabaseExecutionInfoLocationTest` 16/16,
`SimpleMappingExecutionInfoTest` 2/2, `LocalPipelineEngineExecutionIdTest` 1/1,
`BaseCachingExecutionInfoLocationTest` 5/6 (see #5). Blocker 1 reproduced with
a scratch test on `appendLoggingDelta`; blocker 2 established by reading the
stop/finish paths, not by running a stop.
--
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]
