Skip to content

Conversation

lucliu1108
Copy link
Contributor

@lucliu1108 lucliu1108 commented Oct 14, 2025

What

Ticket: https://issues.apache.org/jira/browse/KAFKA-19765
In KIP-1071, there are configurations that affect the assignment, and
that can be configured dynamically.

  • The previous assignment config of streams group is stored as part of
    the StreamsGroupMetadata as a collection of key-value pairs.
  • If the assignment configuration is changed, group epoch is also
    bumped.

Reviewers: Lucas Brutschy [email protected]

@github-actions github-actions bot added triage PRs from the community group-coordinator labels Oct 14, 2025
@lucliu1108 lucliu1108 marked this pull request as ready for review October 14, 2025 20:39
Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I left a lot of comments, but nothing fundamental.

// We bump the group epoch.
int groupEpoch = group.groupEpoch() + 1;
records.add(newStreamsGroupMetadataRecord(group.groupId(), groupEpoch, group.metadataHash(), group.validatedTopologyEpoch()));
records.add(newStreamsGroupMetadataRecord(group.groupId(), groupEpoch, group.metadataHash(), group.validatedTopologyEpoch(), Map.of()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why are dropping the assignment configuration on the floor here? We should use group.assignmentConfiguration here.

@lucasbru lucasbru self-assigned this Oct 15, 2025
@lucasbru lucasbru requested a review from Copilot October 15, 2025 09:32
@lucasbru lucasbru added streams KIP-1071 PRs related to KIP-1071 ci-approved and removed streams labels Oct 15, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements support for storing assignment configurations in Kafka Streams group metadata to enable dynamic configuration changes that affect task assignment. The implementation tracks assignment configurations and bumps the group epoch when these configurations change, ensuring proper rebalancing behavior.

  • Added assignment configuration storage to streams group metadata
  • Implemented group epoch increment when assignment configurations change
  • Updated all test cases to accommodate the new assignment configuration parameter

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
StreamsGroupMetadataValue.json Added AssignmentConfigs field to store assignment configuration parameters as key-value pairs
StreamsGroup.java Added assignmentConfigs field and related methods to track assignment configurations
StreamsCoordinatorRecordHelpers.java Updated newStreamsGroupMetadataRecord to include assignment configurations parameter
GroupMetadataManager.java Added logic to detect assignment config changes and bump group epoch accordingly
StreamsGroupBuilder.java Updated test builder to pass assignment configs parameter
StreamsCoordinatorRecordHelpersTest.java Updated test cases to include assignment configs parameter
GroupMetadataManagerTest.java Updated all test cases and added new test for assignment config changes
Comments suppressed due to low confidence (1)

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:1

  • Extra space before Map.of - should be single space between parameters.
/*

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions github-actions bot removed the triage PRs from the community label Oct 16, 2025
@lucliu1108 lucliu1108 requested a review from lucasbru October 16, 2025 04:09
@lucasbru lucasbru requested a review from Copilot October 16, 2025 06:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 5433 to 5439
streamsGroup.setLastAssignmentConfigs(
value.assignmentConfigs().stream()
.collect(Collectors.toMap(
StreamsGroupMetadataValue.AssignmentConfig::key,
StreamsGroupMetadataValue.AssignmentConfig::value
))
);
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema marks AssignmentConfigs as nullable with a default of null; for older (pre-field) or null-valued records value.assignmentConfigs() will be null, causing a NullPointerException when calling stream(). Guard with a null/empty check (e.g., List<...> configs = value.assignmentConfigs(); if (configs != null) { ... } else set empty) to ensure backward-compatible replay.

Suggested change
streamsGroup.setLastAssignmentConfigs(
value.assignmentConfigs().stream()
.collect(Collectors.toMap(
StreamsGroupMetadataValue.AssignmentConfig::key,
StreamsGroupMetadataValue.AssignmentConfig::value
))
);
if (value.assignmentConfigs() != null) {
streamsGroup.setLastAssignmentConfigs(
value.assignmentConfigs().stream()
.collect(Collectors.toMap(
StreamsGroupMetadataValue.AssignmentConfig::key,
StreamsGroupMetadataValue.AssignmentConfig::value
))
);
} else {
streamsGroup.setLastAssignmentConfigs(Collections.emptyMap());
}

Copilot uses AI. Check for mistakes.

"about": "The hash of all topics in the group." },
{ "name": "ValidatedTopologyEpoch", "versions": "0+", "taggedVersions": "0+", "tag": 0, "default": -1, "type": "int32",
"about": "The topology epoch whose topics where validated to be present in a valid configuration in the metadata." }
"about": "The topology epoch whose topics where validated to be present in a valid configuration in the metadata." },
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'where' to 'were'.

Suggested change
"about": "The topology epoch whose topics where validated to be present in a valid configuration in the metadata." },
"about": "The topology epoch whose topics were validated to be present in a valid configuration in the metadata." },

Copilot uses AI. Check for mistakes.

Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, looks really good. Left 4 minor nits, but this is mostly ready to merge.

{ "name": "ValidatedTopologyEpoch", "versions": "0+", "taggedVersions": "0+", "tag": 0, "default": -1, "type": "int32",
"about": "The topology epoch whose topics where validated to be present in a valid configuration in the metadata." }
"about": "The topology epoch whose topics where validated to be present in a valid configuration in the metadata." },
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, we typically use 2 lines per field in these files

}

@Test
public void testNewStreamsGroupEpochRecordNullGroupId() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should be called testNewStreamsGroupMetadataRecord (sorry, not your change, but while you are at it...)

Objects.requireNonNull(assignmentConfigs, "assignmentConfigs should not be null here");

List<StreamsGroupMetadataValue.AssignmentConfig> assignmentConfigList = new ArrayList<>();
if (!assignmentConfigs.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it's easier to read without this if

"nullableVersions": "0+",
"tag": 1,
"default": null,
"about": "Assignment configuration parameters as key-value pairs."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "Last used assignment configuration..."?

Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes. I missed that we are not handling null in replay.

@lucliu1108 lucliu1108 requested a review from lucasbru October 16, 2025 16:18
Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@lucasbru lucasbru merged commit 36f17cf into apache:trunk Oct 17, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants