Skip to content
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

KAFKA-14462; [6/N] Update Records #13536

Merged
merged 2 commits into from Apr 14, 2023
Merged

KAFKA-14462; [6/N] Update Records #13536

merged 2 commits into from Apr 14, 2023

Conversation

dajac
Copy link
Contributor

@dajac dajac commented Apr 11, 2023

This patch updates the records introduced by KIP-848.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@dajac dajac added the KIP-848 label Apr 11, 2023
Comment on lines 25 to 26
{ "name": "PreviousMemberEpoch", "versions": "0+", "type": "int32",
"about": "The previous member epoch." },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this in the case where the epoch bump does not reach the member. In this case, it will retry with the old epoch and we want to accept it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move some of these comments into the "about" documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Updated those when relevant.

Comment on lines 27 to 28
{ "name": "TargetMemberEpoch", "versions": "0+", "type": "int32",
"about": "The target member epoch." },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this in order to know what was the assignment epoch used to compute the assigned partitions, etc. When if diverges with the current assignment epoch, we know that we need to recompute the assignment.

Comment on lines 29 to 34
{ "name": "AssignedPartitions", "versions": "0+", "type": "[]TopicPartitions",
"about": "The partitions assigned to (owned by) this member." },
{ "name": "PartitionsPendingRevocation", "versions": "0+", "type": "[]TopicPartitions",
"about": "The partitions being revoked by this member." },
{ "name": "PartitionsPendingAssignment", "versions": "0+", "type": "[]TopicPartitions",
"about": "The partitions being assigned to this member." },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the biggest divergence from the KIP. Initially, I wanted to store only the full assignment and to bookkeep those in-memory. I found this extremely confusing during the implementation and error prone as well. It is much better to have three sets instead of ones.

Comment on lines -23 to -24
{ "name": "GroupEpoch", "versions": "0+", "type": "int32",
"about": "The group epoch." },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not necessary because we have a record for the group epoch.

Comment on lines +35 to +38
{ "name": "RebalanceTimeoutMs", "type": "int32", "versions": "0+", "default": -1,
"about": "The rebalance timeout" },
{ "name": "ServerAssignor", "versions": "0+", "nullableVersions": "0+", "type": "string",
"about": "The server assignor to use; or null if not used." },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing fields.

Comment on lines +29 to +30
{ "name": "TopicName", "versions": "0+", "type": "string",
"about": "The topic name." },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing field.

Copy link
Collaborator

@clolov clolov left a comment

Choose a reason for hiding this comment

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

These changes make sense to me. Does it make sense to also update the KIP once this gets merged?

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM

@dajac dajac merged commit 741a273 into apache:trunk Apr 14, 2023
1 check failed
@dajac dajac deleted the KAFKA-14462-6 branch April 14, 2023 12:25
@dajac dajac mentioned this pull request Apr 26, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants