vbhanuchander-lang commented on PR #7745: URL: https://github.com/apache/hop/pull/7745#issuecomment-5153839133
The reorder looks right to me, and the reflection-based test asserting `isDone()` is still false at the moment of the offer is a nice way to pin the ordering itself rather than just the outcome — that's the part a future refactor would otherwise silently undo. One thing I'd raise while this code is under the microscope, because it is the same failure mode this PR is closing. `getArray` is bounded — `new ArrayBlockingQueue<>(BATCHSIZE, true)` — and the two enqueue sites treat that differently: - `putRowWait` (line 99), the full-batch path: `getArray.offer(inputBuffer, time, tu)`, the timed overload, so it waits for space. - `setDone` (line 171), the last-partial-batch path this PR touches: `getArray.offer(inputBuffer)`, the non-blocking overload, whose `false` return is discarded. So if `getArray` is full at the moment `setDone()` runs, the final partial batch is dropped and nothing reports it — the same lost-rows outcome as #7742, reached through a different door, and most likely under the same condition that made the race matter in the first place: a consumer lagging behind the producer. Since every ordinary path reaches `getArray` through `putRowWait`'s timed offer, the plain `offer` in `setDone` looks more like an oversight than a deliberate choice to fail fast there. Would it be worth using the timed overload here as well, to match line 99 — or at minimum logging when the offer returns `false`, so a dropped final batch is not silent? Happy to send that as a separate PR if you would rather keep this one scoped to the reorder. Pre-existing behaviour rather than something this PR introduces, so I do not think it blocks merging. -- 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]
