KAFKA-6718 / A Rack awareness for Kafka Streams#10785
KAFKA-6718 / A Rack awareness for Kafka Streams#10785lkokhreidze wants to merge 28 commits intoapache:trunkfrom
Conversation
| final List<String> rackAwareAssignmentTags = getList(RACK_AWARE_ASSIGNMENT_TAGS_CONFIG); | ||
| final Map<String, String> clientTags = getClientTags(); | ||
|
|
||
| for (final String rackAwareAssignmentTag : rackAwareAssignmentTags) { |
There was a problem hiding this comment.
Not sure if doing validations in postProcessParsedConfig method is okay but couldn't find any better place.
| import static org.apache.kafka.streams.integration.utils.IntegrationTestUtils.safeUniqueTestName; | ||
|
|
||
| @Category({IntegrationTest.class}) | ||
| public class RackAwarenessIntegrationTest { |
There was a problem hiding this comment.
Will try to figure out more test cases, but those should already be good starting point.
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class ClientTagAwareStandbyTaskAssignorTest { |
There was a problem hiding this comment.
Will try to figure out more test cases, but those should already be good starting point.
|
Call for review @vvcephei @cadonna @ableegoldman |
…nAssignorTest test
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.function.BiConsumer; | ||
| import java.util.function.Function; |
There was a problem hiding this comment.
Sorry for the un-related changes :( happy to revert if it adds too much noise during the review.
|
@lkokhreidze Thank you for the PR!
Opening small PRs usually increases the review quality and decreases time to the first review. |
|
Hi @cadonna |
|
As far as I can see from skimming the PR, you could have a PR for the standby task assignors with corresponding unit tests without changing any existing public functionality and without adding any partial public functionality. So I guess, it would be fine to have a PR for the standby task assignors. To give context to the reviewers, you should motivate the change in the PR description. Maybe you can do similar with the subscription changes and with some other aspects of this PR. Probably, it would be good to have the config PR that makes the functionality available to the public last, so that the functionality is never partly publicly available. I do not know the details of #7170, but most people with which I have talked about PR sizes agree that if it is possible to split them into smaller pieces, we should do it. Note, that the title of the PR should start with the issue ID of the JIRA tickets which in this case would be KAFKA-6718. In this way a link to the PR will show up on the ticket. You can also open multiple PRs whose title starts with KAFKA-6718. A link for each PR will then show up in the ticket. |
|
Sounds good, will try to split the PR into smaller chunks. |
lkokhreidze
changed the title
|
Closing this one as the PR is split into multiple smaller PRs. |
2 participants
KIP-708 implementation.
Notable changes:
StreamsConfig. Configurations are guarded by the validation rules and corresponding unit tests are present inStreamsConfigTest.SubscriptionInfoversion bump to10and added necessary fields for encoding client tags.StandbyTaskAssignorabstract class.StandbyTaskAssignorInitializerdecides which of theStandbyTaskAssignorimplementations to use based onAssignmentConfigsAssignmentConfigs#rackAwareAssignmentTagsis present, newClientTagAwareStandbyTaskAssignorwill be chosen which distributes the standby tasks based on client tags and configuredrackAwareAssignmentTagsCommitter Checklist (excluded from commit message)