DanielLeens commented on PR #12281:
URL: https://github.com/apache/seatunnel/pull/12281#issuecomment-5661991396
@saloni-eng thank you for the kind words — really appreciate you sticking
with this through three rounds!
I do need to walk back my "Ready to merge" conclusion from earlier today,
though. @goutamadwant just posted four more findings against the same current
head (`90d798ad`), and I've verified each of them independently in-thread — all
four are real, current issues, not something already resolved by a prior commit:
1. `keep_params_as_form` is documented and consumed by
`SplunkSourceParameter`, but missing from `SplunkSourceFactory.optionRule()` —
real jobs using the documented config fail config validation with "unknown
option keys".
2. The doc's example block name (`Http-Splunk {`) doesn't match the
registered factory identifier (`Splunk`), so the documented example config
can't be parsed as written.
3. The documented default `method: POST` doesn't match the actual code
default (`GET`, inherited from `HttpSourceOptions.METHOD`) — an omitted
`method` silently breaks against Splunk's export endpoint.
4. `SplunkSourceReader.filterAndUnwrapNdjson()` still fully materializes the
response multiple times over rather than streaming it — a real OOM risk on
large exports, reproduced by @goutamadwant with a 30MB response under a 96MB
heap.
No new commit has landed since my last review, so I'm not doing a fresh full
review pass right now — but please treat my earlier "Ready to merge" as
superseded. This PR currently has 4 open blockers (config-validation gap,
doc/factory-identifier mismatch, wrong default, memory/OOM) that need fixing
first. Once a fix is pushed, ping me and I'll do a full re-review of the new
head.
Thanks both for the very thorough back-and-forth on this one — this kind of
scrutiny is exactly what keeps a new connector production-safe before it ships.
--
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]
