davidzollo commented on PR #10454:
URL: https://github.com/apache/seatunnel/pull/10454#issuecomment-4563338663
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 update. I re-reviewed the latest head from scratch and traced
the actual PIT/scroll slicing path again from
`ElasticsearchSourceSplitEnumerator` into `ElasticsearchSourceReader`.
The main blocker from my previous review is fixed on the current head. The
reader now creates the PIT lazily when a split actually starts reading, so the
queued-slice path no longer starts the PIT lifetime too early in the
enumerator. I also checked the new queued-slice E2E path and the scroll-slice
coverage, and they now hit the scenario that was previously missing.
Runtime path I checked:
```text
Split planning
-> ElasticsearchSourceSplitEnumerator.getElasticsearchSplit(...)
-> create one split per slice by slice_id / slice_max
-> no pre-created shared PIT is injected here anymore
Reader path
-> ElasticsearchSourceReader.searchWithPointInTime(...)
-> if the split has no PIT yet, create PIT lazily at first read
-> reuse that PIT only within the active reader path
-> ElasticsearchSourceReader.searchByScroll(...)
-> scroll slicing now passes slice_id / slice_max too
Regression coverage
-> ElasticsearchIT.testElasticsearchWithPITSliceQueued(...)
-> slice_max(4) > parallelism(2) now exercises queued slices explicitly
```
Findings:
- I do not see a reopened source-level blocker on the latest head.
- The earlier shared-PIT lifetime problem is fixed.
- The queued-slice E2E that was missing before is present on the current
head.
Test stability:
- Rating: Stable
- Why: the new PIT coverage now includes the delayed-slice path that
mattered for the previous blocker, and I did not see a new flaky pattern
introduced in the added test methods themselves.
### Conclusion: can merge
Blocking items:
- None from my source-level review on the latest head.
- Please still let the current GitHub `Build` run finish green before merge.
Non-blocking suggestions:
- None beyond keeping an eye on CI for environment-only failures.
--
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]
