-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(producer): rack-aware partitioner #3198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Zorn Hsu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might additionally reorder some things to:
clientRack := client::RackID
var partitionsInRack []int32
for _, p := range partitions {
…
}
if len(partitionsInRack) > 0 {
…
}This would put the clientRack declaration distinctly at the top of the code block, so its loop invariance is more pronounced. As well, all the code dealing with the partitionInRack is grouped more tightly together.
| // setting for the JVM producer. | ||
| Partitioner PartitionerConstructor | ||
| // Controls whether the partitioner is rack-aware. This also affects custom partitioners. | ||
| PartitionerRackAware bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Do we have any Java-side Kafka config value to inform this config name? Does Java’s Kafka even support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the Java version is partitioner.rack.aware, where the Java PR to support this is pending review.
Signed-off-by: Zorn Hsu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Hello @dnwe , would you mind taking a look at this PR when you are available? |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@zornhsu don't worry, we will get to it in time. I had partially been waiting to see what feedback the upstream apache/kafka PR received, but that has still not yet had any review |
|
Thank you for the feedback, @dnwe! To help me plan my work, when do you think you would be able to review this PR? To enhance Sarama feature offerings, I would propose that we independently review and merge the PR in the absence of upstream reviews. The primary dependency appears to be the config name. If the final Java config name is changed, I am willing to submit a follow-up pull request to deprecate the current config and align it with the Java version config. |
|
@ijuma (seeing as I have you nearby) do you have any thoughts on this? The KIP and this PR both some like they provide a reasonable feature and the motivation is valid, but this is an unusual situation where upstream haven’t yet had much discussion nor decided on it, but we have a proposal to add the capability to Sarama – I’m not sure what’s the best course of action here |
|
@dnwe Since this is purely a client-side KIP, you have the option of not waiting for the upstream KIP to land. That said, there is a risk for the upstream KIP to evolve and then you may end up with different behavior. For example, I just commented on the KIP thread asking why we need two configs (which is different from KIP-392 for the consumer). |
|
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
|
Pending @~showuon review the Java version of this PR |
Description
BuiltInPartitioner)Producer.PartitionerRackAware. Together with the existingRackIDconfig, users are free to enable this new behavior.-rackidtokafka-console-producer.goto facilitate manual testing.Why
See KIP-1123 Motivation
Test Plan
Manually tested on 3 node cluster with 3 racks and 3 partitions
rackidmismatch would fall back to random partitioningBackward compatible with no
rackidto random partitioning