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

MINOR: Move DeleteGroupsResult to internals package. #19057

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

Conversation

smjn
Copy link
Contributor

@smjn smjn commented Feb 28, 2025

@github-actions github-actions bot added triage PRs from the community clients small Small PRs labels Feb 28, 2025
@AndrewJSchofield AndrewJSchofield added KIP-932 Queues for Kafka ci-approved and removed triage PRs from the community labels Feb 28, 2025
@AndrewJSchofield
Copy link
Member

@junrao Is this better? It seems to me that we now have two public classes inheriting from a class in the internals package. Would you prefer that we remove the inheritance and just duplicate the code into DeleteConsumerGroupsResult and DeleteShareGroupsResult?

@ijuma
Copy link
Member

ijuma commented Feb 28, 2025

I don't think we should do this change for the reason @AndrewJSchofield mentioned. Parent classes of public classes need themselves to be public.

@junrao
Copy link
Contributor

junrao commented Feb 28, 2025

@AndrewJSchofield: DeleteGroupsResult wasn't included as part of the public interface in the original KIP. Do you think that it makes sense to include it in the public interface?

@AndrewJSchofield
Copy link
Member

@AndrewJSchofield: DeleteGroupsResult wasn't included as part of the public interface in the original KIP. Do you think that it makes sense to include it in the public interface?

To be honest, I think this area is ripe for a bit of rationalisation, not just in terms of these classes. We probably ought to have Admin.deleteGroups(DeleteGroupsOptions) to go alongside Admin.listGroups(ListGroupsOptions), rather than a proliferation of Admin.deleteXXXGroups methods. Then you could specify a group type optionally with the deletion operation, and Admin.deleteShareGroups() would be removed and Admin.deleteConsumerGroups() would be deprecated. I wish I'd realised when I did KIP-1043.

I will discuss with @dajac because I'd like to get him on board before proposing a change like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants