SEZ9 commented on PR #11559: URL: https://github.com/apache/seatunnel/pull/11559#issuecomment-5862884816
Thanks for following up on `PR11559-F1`. - The empty diff on `KafkaSourceReader.java` between `4d75844b4a` and `8aff28b16c` (untouched by `2ab2851f03d`) matches what I expected — no re-review of the production allowlist logic is needed this round. - `e2f3eb344f` addresses the open test-coverage ask. Exercising the real private static `KafkaSourceReader.deserializeSplit` reflectively, rather than the test class's own plain-`ObjectInputStream` helper, is the right move — the earlier `stagedSplitsShouldBeDeduplicatedBySplitId` / `snapshotGateAfterOpenShouldPreserveActivatedSplitsAndNoMoreSplits` assertions never reached `KafkaGateObjectInputStream.resolveClass`, so this closes a real gap. Two small asks before I consider F1 fully closed: 1. **Tighten the negative case.** `productionDeserializeSplitShouldRejectClassOutsideAllowlist` uses `java.util.HashMap`, which a prefix check would reject just as well as an exact-name check. Since the stated intent is to fail a future refactor that widens the allowlist back to a prefix match, please add (or swap in) a `Serializable` class under the same package prefix as the allowlisted split classes that is *not* on the allowlist — that is the only input that actually distinguishes exact-name from prefix matching. 2. **Reflection on a private method.** Reflective access to a private static is fine as a stopgap, but it will break silently on a rename. Either make `deserializeSplit` package-private with a short note that it is exposed for tests, or add a comment in the test explaining why reflection is used, so the next person doesn't "fix" it by reintroducing a local helper. Once those are in, please confirm the two new tests pass locally and I'll do a final pass on the test file only. <!-- streview-comment:1376 --> -- 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]
