KAFKA-19945: Always set status field in StreamsGroupHeartbeat response #21031
Conversation
The status field is now always set in the response, even when empty, to ensure that the status gets reset on the client.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the status field in StreamsGroupHeartbeat responses was conditionally set only when non-empty. Since the field is nullable with a default of null, clients that ignore null values were unable to properly reset/clear the status. The fix ensures the status field is always set in the response, even when it's an empty list.
Key changes:
- Removed conditional check before setting status field in responses
- Added test to verify status field is always present
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java | Removed the if (!returnedStatus.isEmpty()) check, ensuring status is always set on the response even when empty |
| group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java | Added test testStreamsGroupHeartbeatAlwaysSetsStatus to verify status field is always present in responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!returnedStatus.isEmpty()) { | ||
| response.setStatus(returnedStatus); | ||
| } | ||
| response.setStatus(returnedStatus); |
There was a problem hiding this comment.
The code change to always set the status field will break many existing tests that don't expect .setStatus(List.of()) in responses when there are no status messages.
Looking at the test file, there are numerous tests (e.g., testMemberJoinsEmptyStreamsGroup at line 16265, testStreamsGroupHeartbeatPartialResponseWhenNothingChanges at line 17397, testStreamsUpdatingMemberMetadataTriggersNewTargetAssignment at line 16986, and many others) that create expected StreamsGroupHeartbeatResponseData objects without calling .setStatus(), which means the field defaults to null.
After this change, the actual responses will have .setStatus(List.of()) (an empty list), which won't equal null. This will cause test failures. All affected tests need to be updated to include .setStatus(List.of()) in their expected response objects.
|
Overall makes sense to me. Guess we might want to update the KIP for this case, so that the field is not nullable any longer? |
apache#21031) The status field should always set in the response, even when empty, to ensure that the status gets reset on the client. We are not doing this due to an oversight - it is defined as nullable, and it is null when not set. So status does not clear correctly on the client, which ignores the field if it's null. This can cause the streams application to incorrectly timeout, if the source topic does not exist when the application is first started, but it is created after the application started. Otherwise, there is no noticable difference. Reviewers: Lucas Brutschy <[email protected]>
2 participants
The status field should always set in the response, even when empty, to
ensure that the status gets reset on the client.
We are not doing this due to an oversight - it is defined as nullable,
and it is null when not set. So status does not clear correctly on the
client, which ignores the field if it's null.
This can cause the streams application to incorrectly timeout, if the
source topic does not exist when the application is first started, but
it is created after the application started. Otherwise, there is no
noticable difference.
Reviewers: Lucas Brutschy [email protected]