DanielLeens commented on PR #12011:
URL: https://github.com/apache/seatunnel/pull/12011#issuecomment-5476953121
Thanks for the very thorough parallel review, @SEZ9 — you caught real gaps
my own pass missed. I went back to the head commit (`340b2d2c8`) and verified
each finding against the actual source rather than just taking them at face
value; here's where I land.
**Conceding, with evidence — Issue 3 is real and I should have caught it.**
`SnmpTargetFactory.create()` does `target.setAddress(new
UdpAddress(config.getHost() + "/" + config.getPort()))`
(`SnmpTargetFactory.java:35`), and SNMP4J's `UdpAddress(String)` throws an
unchecked `IllegalArgumentException` on a malformed address. Tracing the call
site: `Snmp4jSetClient`'s constructor calls `SnmpTargetFactory.create(config)`
*before* its own `try { snmp.listen(); } catch (IOException e)` block
(`Snmp4jSetClient.java:44-53`), and `SnmpSinkWriter`'s constructor only catches
`IOException` around `clientFactory.create(config)`
(`SnmpSinkWriter.java:44-52`). So a malformed `host` (containing `/`, or an
unresolvable name) does throw a raw, unwrapped `IllegalArgumentException`
straight out of the writer constructor — not a `SnmpConnectorException`, and
not the `IOException` path I checked in my original 1.4 write-up. I focused on
the credential-non-disclosure and retry/timeout wrapping and m
issed this construction-time path entirely. Agreed this is a legitimate Medium
error-handling gap.
**Confirmed valid — Issue 1, and I missed a whole layer.** I checked
in-connector logging (no `Logger`/`toString()` exposure) but not the engine's
job-config log masking. I pulled `ConfigShadeUtils.DEFAULT_SENSITIVE_KEYWORDS`
from `seatunnel-core-starter` directly: it's `{"password", "username", "auth",
"token", "access_key", "secret_key"}`, matched by exact key equality
(`Map.computeIfPresent(sensitiveOption, ...)`, not substring). `community`
isn't in that list, so the resolved job config — including the plaintext
community string — will render unmasked wherever the engine logs/prints the
parsed config at submission, which does contradict the doc's "does not write
this value to its logs" claim at that layer (the docs are accurate about the
*connector's own* code, just not about the engine's default log-masking
coverage). Good catch — this deserves either a `shade.options`-based fix or, at
minimum, an explicit doc caveat that `community` isn't auto-masked by default
and u
sers should add it to `shade.options` themselves.
**Pushing back with evidence — Issue 2's drift risk doesn't apply to the
current code.** I checked `SnmpSourceOptions.java` directly: it doesn't
redeclare `PORT`/`TIMEOUT_MILLIS`/`RETRIES` with its own values, it does
`public static final Option<Integer> PORT = SnmpOptions.PORT;` (and the same
for `HOST`/`COMMUNITY`/`TIMEOUT_MILLIS`/`RETRIES`) — literally the same
`Option` object reference, not a second copy of the same value. There's nothing
to drift between; a future edit to `SnmpOptions.PORT`'s default changes both
source and sink identically, by construction, not "if someone remembers to keep
them in sync." That said, the underlying suggestion — an explicit default-value
assertion in `SnmpSourceConfigTest` — is still cheap, harmless insurance
against a future refactor that breaks that reference, so I'd keep it as a
nice-to-have even though I don't think the risk is live today.
**Agreed, all reasonable — Issues 4-8.** Issue 4 (no e2e module) is accurate
— I confirmed there's no `connector-snmp-e2e` anywhere for either the source or
this new sink, so real engine-path coverage (SPI discovery via
`plugin-mapping.properties`, factory option validation under Zeta) is genuinely
absent; worth noting this mirrors the pre-existing source connector's own test
strategy rather than being a new gap introduced here, but that's not a reason
to leave it uncovered forever. Issue 5 is a good sharpening of something I only
described qualitatively in my 1.3 ("throughput ceiling") — the concrete
`timeout_millis * (retries + 1)` = 10s-by-default blocking formula and its
checkpoint-timeout implication on streaming engines is worth calling out
explicitly in the docs. Issues 6, 7 and 8 are all correct, low-risk doc fixes
(missing `common-options` row despite the example using `plugin_input`, the
undocumented SNMP4J-level retransmission risk for non-idempotent OIDs, and the
unexplained `${SNMP_COMMUNITY}` substitution in the example).
Net effect on my recommendation: my original "Ready to merge" was too quick
— Issues 1 and 3 are genuine, Medium-severity gaps I should have caught
(credential-masking coverage and an unwrapped construction-time exception), and
I'd now say those two plus the e2e gap (Issue 4) are worth addressing before
merge rather than as pure follow-ups, even though none of them are
High/blocking on their own. Issue 2 I'd downgrade to a non-issue given the
shared-reference evidence above, though the extra test assertion is still
welcome. Thanks again for catching what I missed — happy to see all of this
land in this PR.
--
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]
