sun-9545sunoj commented on PR #12463:
URL: https://github.com/apache/seatunnel/pull/12463#issuecomment-5838091191
Thanks so much for the incredibly detailed and thorough review! I really
appreciate you catching that scoping gap with the threshold and the NPE in the
CI logs.
I've just pushed a new commit that addresses all of your feedback:
1. Issue 1 (NPE in CI): Fixed! I updated the test to pass a real
io.debezium.relational.Column instance (using
Column.editor().name("id").create()) instead of null. The decision logic is now
properly
exercised in the test without throwing an NPE.
2. Issue 2 (Gating Threshold): Excellent catch on the
sampleShardingThreshold default negating the fix for tables under ~8.1M rows. I
went with your Option A: I removed the sampleShardingThreshold
dependency for the gap-skewed branch and changed it to just check
shardCount > 1. (The original threshold logic remains intact for the fallback
"uneven" branch).
3. Issue 6 (Reflection Brittleness): I promoted splitTableIntoChunks from
private to protected (subclass visibility). This allowed me to strip out the
brittle Method.setAccessible reflection in the test
and safely invoke it directly.
4. Issue 7 (Unrelated tests): Just for context, the enableConcurrentRead
tests were pre-existing in this file prior to my PR. I've left them as is to
avoid regressing existing test coverage, though I
agree they are orthogonal to this specific bug.
I've verified the tests locally and the CI should hopefully be completely
green on this run. Thanks again for the guidance—let me know if you spot
anything else!
--
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]
