You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "smjn (via GitHub)" <gi...@apache.org> on 2025/02/28 10:50:59 UTC
[PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
smjn opened a new pull request, #19057:
URL: https://github.com/apache/kafka/pull/19057
* `DeleteGroupsResult` was introduced in the PR https://github.com/apache/kafka/pull/18928 to serve as common parent class of `DeleteConsumerGroupsResult` and `DeleteShareGroupsResult` since the impl of the child classes is almost identical.
* However, since `DeleteGroupsResult` is abstract and not for direct consumption, it makes sense to not keep it in public packages per the comment https://github.com/apache/kafka/pull/18928#discussion_r1974106703.
* In this PR, we perform this refactor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2690555953
@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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2691179820
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1981910462
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
@chia7712 had suggested to remove it saying the interface is mature.
https://github.com/apache/kafka/pull/19057#issuecomment-2698014961
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2698210918
@AndrewJSchofield @chia7712
Deleted common class. Duplicated methods of consumer within share.
Please add CI tag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1981904870
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
That would be consistent with the rest of the KIP-932 interfaces, although not that critical in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2696438683
Could we use "composite" instead of "inheritance"?
1. move `DeleteGroupsResult` into internal package
2. add `delegate: DeleteGroupsResult` to `DeleteConsumerGroupsResult` and `DeleteShareGroupsResult`
```java
private final DeleteGroupsResult delegate;
public DeleteConsumerGroupsResult(final Map<String, KafkaFuture<Void>> futures) {
this.delegate = new DeleteGroupsResult(futures);
}
```
With this approach, we can remove `DeleteGroupsResult` from public APIs and maximize its internal reuse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1982945047
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
@chia7712
@ijuma
Added requisite annotation https://github.com/apache/kafka/pull/19133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2691225538
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2698208660
I agree that the logic we are trying to share is so simple that we don't need a public base class. If it's helpful, we could add internal utility methods that can be used by many of these result classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1981900730
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
Do we need to preserve the Evolving tag?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2697565099
> Could we use "composite" instead of "inheritance"?
>
> 1. move `DeleteGroupsResult` into internal package
> 2. add `delegate: DeleteGroupsResult` to `DeleteConsumerGroupsResult` and `DeleteShareGroupsResult`
>
> ```java
> private final DeleteGroupsResult delegate;
> public DeleteConsumerGroupsResult(final Map<String, KafkaFuture<Void>> futures) {
> this.delegate = new DeleteGroupsResult(futures);
> }
> ```
>
> With this approach, we can remove `DeleteGroupsResult` from public APIs and maximize its internal reuse.
Good idea, @chia7712. We can move this forwards like this, and the potential rationalisation step can be separate. @smjn, seems good to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2697630616
> > Could we use "composite" instead of "inheritance"?
> >
> > 1. move `DeleteGroupsResult` into internal package
> > 2. add `delegate: DeleteGroupsResult` to `DeleteConsumerGroupsResult` and `DeleteShareGroupsResult`
> >
> > ```java
> > private final DeleteGroupsResult delegate;
> > public DeleteConsumerGroupsResult(final Map<String, KafkaFuture<Void>> futures) {
> > this.delegate = new DeleteGroupsResult(futures);
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > With this approach, we can remove `DeleteGroupsResult` from public APIs and maximize its internal reuse.
>
> Good idea, @chia7712. We can move this forwards like this, and the potential rationalisation step can be separate. @smjn, seems good to you?
Hi,
Thanks @chia7712 and @AndrewJSchofield.
Composition will definitely be better (low coupling) but in this specific case the methods being identical, I do not see it adding value. We will be copying the methods and have a single impl line with `super.method`. It would have been worthwhile if we had a couple of specialized methods in each class.
I would rather go with copying the methods over this and eliminate `DeleteGroupsResult`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1982945047
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
@chia7712
@ijuma
https://github.com/apache/kafka/pull/19133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2690601588
I don't think we should do this change for the reason @AndrewJSchofield mentioned. Parent classes of public classes need themselves to be public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Move DeleteGroupsResult to internals package. [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #19057:
URL: https://github.com/apache/kafka/pull/19057#issuecomment-2698014961
> I would rather go with copying the methods over this and eliminate DeleteGroupsResult.
that is totally fine to me.
Furthermore, `DeleteConsumerGroupsResult` is now a stable API, so it's strange that it extends an "evolving" `DeleteGroupsResult`. It seems to me that implies `DeleteGroupsResult` is also stable. Perhaps we can copy the methods as a temporary solution and merge them into 4.0 to avoid exposing public API unintentionally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1982857330
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
That seems wrong for `DeleteShareGroupsResult` given that queues is still EA. I think @chia7712 was not talking about this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1982864874
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
Oh, I was talking about `DeleteConsumerGroupsResult`, not `DeleteShareGroupsResult`. Sorry for any confusion I caused in this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1982857330
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
That seems wrong given that queues is still EA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #19057:
URL: https://github.com/apache/kafka/pull/19057#discussion_r1982871587
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -20,13 +20,33 @@
import org.apache.kafka.common.KafkaFuture;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
/**
* The result of the {@link Admin#deleteShareGroups(Collection <String>, DeleteShareGroupsOptions)} call.
*/
-public class DeleteShareGroupsResult extends DeleteGroupsResult {
+public class DeleteShareGroupsResult {
Review Comment:
Will rectify, thanks for pointing out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Delete DeleteGroupsResult class. [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield merged PR #19057:
URL: https://github.com/apache/kafka/pull/19057
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org