SEZ9 commented on PR #12011:
URL: https://github.com/apache/seatunnel/pull/12011#issuecomment-5578163188
Thanks @DanielLeens for re-reviewing `f74a0d1609c` end to end rather than
just the delta from `cc7658b6`.
- Declarative option validation: agreed, your inline note confirms
`SnmpSinkFactory.optionRule()` and the five new `SnmpSinkFactoryTest` cases. I
consider that item closed.
- `community` / `shade.options` doc wording: noted that you consider this
closed on this head as well.
- CI on this exact commit: agreed this is the only gating item. I'll hold
off on a merge decision until CI has completed on `f74a0d1609c`.
Two small asks:
1. Your review body appears to be cut off mid-sentence ("...verified ev").
Could you re-post the rest of section 1.1 and anything after it?
2. Earlier rounds also covered the `SnmpTargetFactory` host/UDP address
handling, the synchronous per-row SET blocking on unreachable agents, the
missing e2e case for the sink registered in `plugin-mapping.properties`, and
the `docs/en/connectors/sink/SNMP.md` items (common-options row,
`${SNMP_COMMUNITY}` resolution, retries/duplicate-delivery note). Could you
confirm per item which were verified as addressed on `f74a0d1609c` and which
were intentionally deferred? Your summary says every follow-up is closed, but
given the truncation I'd rather have it stated explicitly.
Once CI is green on this head and I have the per-item confirmation, I'm
happy to move forward.
<!-- streview-comment:886 -->
--
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]
