DanielLeens commented on PR #10885:
URL: https://github.com/apache/seatunnel/pull/10885#issuecomment-4518060421
Thanks for the contribution. I pulled the latest head
`c1bb7f8106e308e34e1f16c5962985067ac101d7` locally as `seatunnel-review-10885`,
reviewed the full diff against `apache/dev`, and retraced the real Kafka sink
writer -> producer -> partitioner lifecycle.
# What this PR fixes
- User pain: `MessageContentPartitioner` used a static global
`assign_partitions` state, so one Kafka sink job / producer instance could leak
its routing rules into another.
- Fix approach: remove the static global field, pass the configured
partitions through producer properties, and let each partitioner instance read
them from `configure(...)`.
- In one sentence: this makes the partition-routing rules instance-scoped
again instead of JVM-global.
# Runtime path I checked
```text
Kafka sink writer initialization
-> KafkaSinkWriter.createKafkaProducer()
-> sets PARTITIONER_CLASS_CONFIG = MessageContentPartitioner
-> injects assign.partitions into producer configs
Kafka producer creates partitioner instance
-> MessageContentPartitioner.configure(configs)
-> the current instance stores its own assignPartitions list
Message routing
-> MessageContentPartitioner.partition(...)
-> match configured keywords first
-> otherwise fall back to hashing over the remaining partitions
```
# Code review
I do not see a new source-level blocker on this head.
The important part here is that the fix now matches Kafka's real lifecycle
model. The partitioner configuration is no longer written into a static global
field from `KafkaSinkWriter`; it is carried as producer config and read per
partitioner instance in `configure(...)`. That is exactly what prevents
cross-job / cross-producer interference.
The new unit test coverage is also aligned with the actual regression
surface:
- cross-instance isolation
- explicit assigned-partition routing
- fallback hashing for unassigned messages
# Compatibility
This is backward compatible from a user-config perspective. The config name
does not change. The behavioral change is only the bug fix: separate producer
instances no longer corrupt each other's partition-routing state.
# Performance / side effects
No meaningful CPU/memory concern here. This is effectively a state-scoping
fix with negligible overhead.
# Testing coverage and stability
The new tests look stable to me: no sleeps, no timing-sensitive assertions,
no external resource races, and no shared static state left in the test path.
# Merge conclusion
### Conclusion: can merge
1. Blocking items
- I did not find a blocking issue on the current head.
2. Suggested follow-up items
- None.
Overall, this is a focused and correct fix. From Daniel's side, I don't see
a source-level reason to hold it back.
--
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]
