Skip to content

KAFKA-14691; [1/N] Add new fields to OffsetFetchRequest and OffsetFetchResponse - WIP#13764

Closed
clolov wants to merge 3 commits intoapache:trunkfrom
clolov:kafka-14691
Closed

KAFKA-14691; [1/N] Add new fields to OffsetFetchRequest and OffsetFetchResponse - WIP#13764
clolov wants to merge 3 commits intoapache:trunkfrom
clolov:kafka-14691

Conversation

@clolov
Copy link
Copy Markdown
Contributor

@clolov clolov commented May 25, 2023

This is part 1 of KAFKA-14691. I change the JSON files which are used to generate the POJOs used during requests and responses.

@clolov clolov changed the title KAFKA-14691; [1/N] Add new fields to OffsetFetchRequest and OffsetFet… KAFKA-14691; [1/N] Add new fields to OffsetFetchRequest and OffsetFetchResponse May 25, 2023
@clolov clolov requested a review from dajac May 25, 2023 15:37
@clolov clolov added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label May 25, 2023
Copy link
Copy Markdown

@Hangleton Hangleton left a comment

Choose a reason for hiding this comment

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

Thanks @clolov for the PR. Also tagging @jolshan who had started to look at the same a while ago.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: The "about" field could be on a new line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we want to add these fields in this PR?

@jolshan
Copy link
Copy Markdown
Member

jolshan commented May 25, 2023

Since this is still under development, can we set the flag to indicate the latest version is unstable?

@dajac
Copy link
Copy Markdown
Member

dajac commented May 26, 2023

I have a few comments/questions:

  • I am not really comfortable with merging this without the server side implementation. @clolov Is there a strong reason to not do them together?
  • I agree with @Hangleton that it may be better to start with adding the TopicId only. This is complicated enough on its own. We can the other fields afterwards.
  • I agree with @jolshan that we should set "latestVersionUnstable": true while in development.

@clolov
Copy link
Copy Markdown
Contributor Author

clolov commented May 30, 2023

I have a few comments/questions:

1. I am not really comfortable with merging this without the server side implementation. @clolov Is there a strong reason to not do them together?

2. I agree with @Hangleton that it may be better to start with adding the TopicId only. This is complicated enough on its own. We can the other fields afterwards.

3. I agree with @jolshan that we should set `"latestVersionUnstable": true` while in development.

I will accommodate 2 and 3 in subsequent commits. There was no reason for 1 other than splitting the pull request into nicer to review chunks. I am happy with trying to put Request/Respone changes + server side changes + tests in the same pull request similar to #13240.

Thank you all for the reviews!

@clolov clolov changed the title KAFKA-14691; [1/N] Add new fields to OffsetFetchRequest and OffsetFetchResponse KAFKA-14691; [1/N] Add new fields to OffsetFetchRequest and OffsetFetchResponse - WIP Jun 7, 2023
@dajac dajac added the stale Stale PRs label Sep 5, 2023
@dajac
Copy link
Copy Markdown
Member

dajac commented Sep 29, 2023

Closing this PR for now as the topic id work will be done later. We can re-open it when we resume the work.

@dajac dajac closed this Sep 29, 2023
@clolov clolov deleted the kafka-14691 branch January 27, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

KIP-848 The Next Generation of the Consumer Rebalance Protocol stale Stale PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants