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-18887 Implement Streams Admin APIs #19049

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

aliehsaeedii
Copy link
Contributor

@github-actions github-actions bot added triage PRs from the community streams clients labels Feb 27, 2025
Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @aliehsaeedii ! I made a pass on the production code.

import java.util.Map;

/**
* The result of the {@link AdminClient#alterConsumerGroupOffsets(String, Map)} call.
Copy link
Member

Choose a reason for hiding this comment

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

This is the result of alterStreamsGroupOffsets

* The API of this class is evolving, see {@link Admin} for details.
*/
@InterfaceStability.Evolving
public class DeleteStreamsGroupOffsetsResult extends DeleteConsumerGroupOffsetsResult {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should use inheritance here. There is no subtyping relationship here really. Also, technically this is a change to the public API that needs to be changed in the KIP.

I understand you want to avoid reimplementing the class. Have you considered using delegation? That is, the (package-protected) contructor for this class takes a DeleteConsumerGroupOffsetsResult and delegates calls to partitionResult and all to it?

* The API of this class is evolving, see {@link AdminClient} for details.
*/
@InterfaceStability.Evolving
public class AlterStreamsGroupOffsetsOptions extends AlterConsumerGroupOffsetsOptions {
Copy link
Member

Choose a reason for hiding this comment

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

I would directly derive from AbstractOptions<AlterStreamsGroupOffsetsOptions> here. Similar for the other option classes.

@@ -945,6 +945,29 @@ default ListConsumerGroupOffsetsResult listConsumerGroupOffsets(Map<String, List
return listConsumerGroupOffsets(groupSpecs, new ListConsumerGroupOffsetsOptions());
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

For all the javadocs for methods in the Admin, I'd suggest we add a @note that this effectively does the same as the corresponding consumer group method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients streams triage PRs from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants