[Fix][Connector-V2][Http] Stop cursor pagination when cursor does not advance#10944
[Fix][Connector-V2][Http] Stop cursor pagination when cursor does not advance#10944officialasishkumar wants to merge 1 commit into
Conversation
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the fix. I reviewed the latest head from scratch and retraced the bounded HTTP cursor-pagination path. The change is focused, and I did not find a new blocker on the current revision.
What This PR Fixes
- User pain: some cursor-based HTTP APIs signal the terminal page by repeating the last cursor instead of returning an empty cursor. The old reader only stopped on an empty cursor, so it could keep polling forever without progress.
- Fix approach: the latest head treats a non-empty but unchanged cursor as "not advanced" and stops the bounded reader.
- One-line summary: this is a precise fix for an actual no-progress polling loop in the cursor path.
Full Runtime Chain
internalPollNext()
-> pollAndCollectData(...)
-> collect(output, data) [384-433]
-> cursor pagination branch [397-417]
-> parse next cursor from JSON
-> compare with previous cursor
-> if empty or unchanged => noMoreElementFlag = true
-> finally block
-> bounded source + noMoreElementFlag => signalNoMoreElement()
Key Findings
- The normal bounded cursor-pagination path definitely hits this logic.
- The new stop condition matches the real no-progress scenario from the PR description.
- The regression test verifies both the bounded stop and the fact that the reader does not keep issuing requests forever.
- I did not find a new source-level blocker on the current head.
Findings
No new blocking issues found on the current head.
Merge Conclusion
Conclusion: can merge
- Blocking items
- None from Daniel on the current revision.
- Suggested non-blocking follow-up
- The top-level
Buildsignal still looks freshly unsettled, so I would let the latest run finish before merging.
Overall, this looks like a clean fix for a real bounded-reader edge case.
|
Please enable CI check following this instruction |
|
Thanks for the CI follow-up. From the code review side, I do not have any additional source-level blockers on the current head. The next step here is to enable / rerun the workflow and let the Build result settle. Once the contributor pushes a new commit or there is a new technical update, I am happy to take another look. |
Purpose of this pull request
Fixes #10929.
This PR stops HTTP cursor pagination when a response returns the same non-empty cursor as the previous request. The existing cursor path only stopped when the next cursor was empty, so APIs that signal the terminal page by repeating the final cursor could keep polling without progress.
Does this PR introduce any user-facing change?
Yes. HTTP cursor pagination now stops when the cursor does not advance, avoiding repeated requests for cursor APIs that return the final cursor again at end-of-data.
How was this patch tested?
JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 MAVEN_OPTS=-Xmx1536m ./mvnw -B -pl seatunnel-connectors-v2/connector-http/connector-http-base -DskipIT=true -D"license.skipAddThirdParty"=true -D"skip.ui"=true spotless:applyJAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 MAVEN_OPTS=-Xmx1536m ./mvnw -B -U -pl seatunnel-connectors-v2/connector-http/connector-http-base -Dtest=org.apache.seatunnel.connectors.seatunnel.http.HttpSourceReaderInternalPollNextTest -DfailIfNoTests=false -DskipIT=true -D"license.skipAddThirdParty"=true -D"skip.ui"=true testCheck list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.