mattcasters opened a new pull request, #7745: URL: https://github.com/apache/hop/pull/7745
## Summary Fixes [#7742](https://github.com/apache/hop/issues/7742): potential data loss in `BlockingBatchingRowSet.setDone()` due to a race between setting the done flag and enqueueing the last partial batch. ### What was wrong `BlockingBatchingRowSet` batches rows into buffers and only exposes full buffers on `getArray`. When the producer finishes with a **partial** buffer, `setDone()` must flush that last batch. The previous order was: 1. `super.setDone()` → `done` becomes `true` immediately 2. Then offer the last partial batch to `getArray` Downstream consumers (`BaseTransform.getRow()`) treat `isDone() == true` plus an empty poll as end-of-stream and remove the rowset. Under concurrency a consumer could therefore exit **before** the last rows were offered, dropping them. This only applies when batching rowsets are enabled (`HOP_BATCHING_ROWSET=Y`); the default path uses `BlockingRowSet`, which does not defer rows in the same way. ### Fix Reorder `setDone()` so that: 1. The last partial batch is offered to `getArray` (same null-padding as before) 2. `putArray` is cleared 3. Only then is `super.setDone()` called Any thread that observes `isDone()==true` is then guaranteed that the final partial batch (if any) was already published. ## How it was reproduced / tested ### Deterministic reproduction (unit test) `BlockingBatchingRowSetTest.testSetDoneEnqueuesPartialBatchBeforeDoneFlag` instruments `getArray.offer` via reflection and asserts that when the last partial batch is offered, `isDone()` is still **false**. - With the **old** order: test fails (`done` is already true at offer time) - With the **new** order: test passes ### Additional coverage | Test | What it checks | |------|----------------| | `testSetDoneEnqueuesPartialBatchBeforeDoneFlag` | Ordering invariant for issue #7742 | | `testSetDoneWithNoPartialBatchOnlyMarksDone` | No spurious offer when nothing was put | | `testSetDoneAfterFullBatchOnlyMarksDone` | No re-offer when the last put already published a full batch | | `testPartialBatchDrainableAfterSetDone` | All partial-batch rows are readable after `setDone()` | | `testNoDataLossOnConcurrentSetDone` | Producer + BaseTransform-style consumer over 200 iterations; row count always matches | ```bash ./mvnw -pl core -Dtest=BlockingBatchingRowSetTest test ``` ## Reviewer notes 1. **Scope is intentionally small** — only `BlockingBatchingRowSet.setDone()` ordering and tests. No changes to `BaseTransform` or `BlockingRowSet`. 2. **Please focus review on** happens-before semantics: after `setDone()`, `isDone()==true` must imply the last partial batch is already in `getArray` (or there was nothing to flush). 3. The white-box ordering test is the strongest regression guard; the concurrent test is belt-and-suspenders and models the real consumer loop (`getRowWait` → if null and `isDone`, short retry → exit). 4. Optional manual stress: enable `HOP_BATCHING_ROWSET=Y` and run a multi-copy pipeline that ends on a non-multiple of the batch size under load. Default installs do not use this rowset. ## Related issue Closes #7742 -- 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]
