-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-19233: Add tests for duplicate heartbeat request handling #21319
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
Conversation
This adds tests to validate that duplicate full heartbeat requests are handled idempotently in all member states (STABLE, UNREVOKED_PARTITIONS, UNRELEASED_PARTITIONS). Each test verifies that the duplicate produces the same response with no records generated.
|
|
||
| assertEquals(MemberState.STABLE, context.consumerGroupMemberState(groupId, memberId)); | ||
|
|
||
| // Duplicate heartbeat with same request but epoch is now stale. |
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.
nit: The comment is a bit misleading. Would it be better to say Duplicate heartbeat with same request but with the new epoch.
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.
agree is confusing, but actually if the intention is to testDuplicateFullHeartbeatWithRevocationAck I guess the comment is right but the epoch 101 is wrong (should be 100).
Shouldn't we just remove the duplicateRequest and send the same fullRequest from above?
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.
fixed it.
lianetm
left a comment
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.
Thanks! Just one comment
|
|
||
| assertEquals(MemberState.STABLE, context.consumerGroupMemberState(groupId, memberId)); | ||
|
|
||
| // Duplicate heartbeat with same request but epoch is now stale. |
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.
agree is confusing, but actually if the intention is to testDuplicateFullHeartbeatWithRevocationAck I guess the comment is right but the epoch 101 is wrong (should be 100).
Shouldn't we just remove the duplicateRequest and send the same fullRequest from above?
lianetm
left a comment
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.
Thanks! LGTM
This patch adds tests to validate that duplicate full heartbeat requests
are handled idempotently in all member states (STABLE,
UNREVOKED_PARTITIONS, UNRELEASED_PARTITIONS).
Reviewers: Dongnuo Lyu dlyu@confluent.io, Lianet Magrans
lmagrans@confluent.io