davidzollo commented on PR #10879:
URL: https://github.com/apache/seatunnel/pull/10879#issuecomment-4563338939
Re-posting this under Daniel because the previous review submission from my
environment was attributed incorrectly. Please treat this Daniel-authored
comment as the authoritative maintainer review for the current head.
Thanks for the follow-up. I re-reviewed the latest head from scratch against
the current Phase 2 runtime path, and I focused on the latest delta on top of
the previously approved file-collector fixes.
From the source/runtime side, the current head still looks acceptable to me.
The file-collector state-cleanup fixes I previously checked are still in place,
and the newest delta mainly disables the Unix-style rotation regression on
Windows, which matches the real platform constraint for those tests. I do not
see a reopened blocker in the `FileCollectReader` / runtime scheduler / WAL
flow on the latest head.
Runtime path I checked:
```text
File collector path
-> FileCollectReader.readLines(...)
-> on rotation or skip-on-error, clearFileState(...) drops stale
multiline state
-> FileCollectReader.closeInactiveCursors(...)
-> inactive cursor cleanup also clears file-local buffers
Regression coverage
-> FileCollectReaderMultilineTest.rotationResetsMultilineBuffer(...)
->
FileCollectReaderBehaviorTest.skipOnErrorCleansStateAndAllowsRediscovery(...)
-> latest delta disables the Unix-specific rotation case on Windows only
```
Findings:
- I do not see a reopened source-level blocker on the latest head.
- The earlier file-collector state-leak blockers remain fixed.
- The latest Windows-specific test gating matches the real filesystem
constraint rather than hiding a functional regression.
Test stability:
- Rating: Stable
- Why: the current regression tests still use explicit readiness polling,
and the newest change only narrows the Unix-style rotation assertions away from
Windows where that behavior is not representable.
### Conclusion: can merge
Blocking items:
- None from my source-level review on the latest head.
- Please still wait for the current GitHub `Build` run to finish green
before merge.
Non-blocking suggestions:
- None beyond keeping the final PR summary focused on the actual edge-agent
business delta instead of the unrelated upstream base noise.
--
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]
