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

KIP-455: Implement alterPartitionReassignments/listPartitionReassignments admin methods #1419

Merged
merged 21 commits into from Jul 25, 2022

Conversation

jonathanhaviv
Copy link
Contributor

@jonathanhaviv jonathanhaviv commented Jul 20, 2022

This PR adds the protocols and methods for alterPartitionReassignments and listPartitionReassignments referenced in #1181. There are also a few updates to the encoder and decoder for working with the types added by KIP-482:

  • writeUVarIntArray was updated to be able to encode a null array as per the documentation.
  • readUVarIntString was updated to slice from the buffer the length of the bytes - 1 (the length of the string) and move the offset based on the length of the string instead of the byte length since the byte length was the length of the string + 1.

One note on tagged fields. Requests and responses with tagged fields have them in two places:

  • At the top level of the request/response
  • Wherever there is a TAG_BUFFER

The implication of this is that if there are no tagged fields a zero byte needs to be written/read in multiple places depending on the rest of the request/response.

Hope that helps with how to handle any protocols that use the tagged fields or compact types.

@jonathanhaviv jonathanhaviv changed the title KIP-455: Implement alterPartitionReassignments/listPartitionReassignments admin methods #1181 KIP-455: Implement alterPartitionReassignments/listPartitionReassignments admin methods Jul 20, 2022
@jonathanhaviv jonathanhaviv marked this pull request as ready for review July 20, 2022 01:50
Copy link
Collaborator

@Nevon Nevon left a comment

Choose a reason for hiding this comment

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

I'm afraid I haven't had time to go through this really thoroughly yet, as I'm busy with my real job, but there are a couple of things that will need to be looked at. Most importantly the handling of the tagged fields in the response header, since the current implementation won't work if we receive any tagged field, even though the API version indicates that we support them.

types/index.d.ts Outdated Show resolved Hide resolved
docs/Admin.md Outdated Show resolved Hide resolved
src/admin/index.js Outdated Show resolved Hide resolved
src/protocol/encoder.js Show resolved Hide resolved
Copy link
Collaborator

@Nevon Nevon left a comment

Choose a reason for hiding this comment

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

Looks good. I just have a question about what looks like a bug in reading uvarintbytes, and then we're good to go.

types/index.d.ts Outdated Show resolved Hide resolved
src/protocol/decoder.js Show resolved Hide resolved
src/protocol/decoder.js Show resolved Hide resolved
Copy link
Collaborator

@Nevon Nevon left a comment

Choose a reason for hiding this comment

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

Just noticed the mixup of nullable and non-nullable fields.

@Nevon Nevon merged commit e70ee34 into tulios:master Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants