DanielLeens commented on PR #11028: URL: https://github.com/apache/seatunnel/pull/11028#issuecomment-5412546691
Thanks for the thorough follow-up, @SEZ9 - and for catching this within 25 minutes of my approval. I went back and traced Issue 1 end to end before responding, since it directly contradicts what I approved. **Issue 1 (`pageInfo`/`binaryMode` dropped in `ShopifySource.createReader()`) - confirmed, this is a real blocker and my approval was wrong to let it through.** Trace on the current head (`27a3a89b4c0c60c8ddde3a9e9a89e06b4b7c8d14`): - Base `HttpSource.createReader()` builds the reader with the full 8-arg constructor: `httpParameter, readerContext, deserializationSchema, jsonField, contentField, pageInfo, binaryMode, binaryChunkSize`. - `ShopifySource.createReader()` (`ShopifySource.java:44-52`) overrides this and calls the 5-arg constructor instead - no `pageInfo`, no `binaryMode`/`binaryChunkSize` - which delegates through the null-`pageInfo` overload (`HttpSourceReader.java:86-100`), leaving `pageInfoOptional = Optional.empty()`. - I then traced `HttpSourceReader.internalPollNext()` (`HttpSourceReader.java:410-435`): when `pageInfoOptional` is empty, it calls `pollAndCollectData()` exactly once per `pollNext()` invocation, and since `noMoreElementFlag` defaults to `true` (line 76) and is only ever mutated inside the page-aware branch of `collect()` (line 462), on a `BOUNDED` job the `finally` block sees `noMoreElementFlag == true` right after that first request and immediately calls `context.signalNoMoreElement()`. So this is not a hang or an infinite-poll bug - it is exactly what you described: a clean, "successful" job that silently stops after page 1. Since `getHttpBuilder()` still exposes `pageing` in `ShopifySourceFactory.optionRule()` (so a user configuring it hits no validation error, just silently-ignored config), and Shopify caps pages at 250 records, this is a genuine silent-truncation correctness bug on the real data path, not a docs nit. I'm treating my APPROVED review from earlier today as superseded - this needs `pageInfo`/`binaryMode` wired through (or a Shopify-specific reader override) before merge. **Issue 7 (missing `getSourceClass()` override) - also confirmed.** `HttpSourceFactory.getSourceClass()` returns `HttpSource.class`, and `ShopifySourceFactory` does not override it, so factory metadata for the `Shopify` identifier reports the wrong class versus what `createSource()` actually instantiates. Agreed this is Medium on its own, but worth fixing alongside Issue 1 while the file is being touched. I have not independently re-traced Issues 2-6 and 8 line by line in this pass, but they read as consistent with the doc/config-contract and secret-handling gaps I was already tracking in earlier rounds, and I do not have a reason to push back on any of them. To be clear on status: nothing has changed on this head since my (now superseded) approval, so all of your points remain open. I will do a full re-review once a new revision addressing Issue 1 (and ideally Issue 7) is pushed. -- 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]
