dajac commented on code in PR #18034:
URL: https://github.com/apache/kafka/pull/18034#discussion_r1870988775
##########
clients/src/main/java/org/apache/kafka/clients/admin/ConsumerGroupDescription.java:
##########
@@ -108,7 +113,8 @@ public ConsumerGroupDescription(String groupId,
GroupState groupState,
Node coordinator,
Set<AclOperation> authorizedOperations) {
- this(groupId, isSimpleConsumerGroup, members, partitionAssignor,
GroupType.CLASSIC, groupState, coordinator, authorizedOperations);
+ this(groupId, isSimpleConsumerGroup, members, partitionAssignor,
GroupType.CLASSIC, groupState, coordinator, authorizedOperations,
+ Optional.empty(), Optional.empty());
}
public ConsumerGroupDescription(String groupId,
Review Comment:
Was this constructor introduced in 4.0 only? It seems to be the case but we
need to double check. If it is not, we would need to introduce a new one in
order to not break existing usages.
##########
clients/src/main/java/org/apache/kafka/clients/admin/MemberDescription.java:
##########
@@ -30,21 +30,26 @@ public class MemberDescription {
private final String host;
private final MemberAssignment assignment;
private final Optional<MemberAssignment> targetAssignment;
+ private final Optional<Integer> memberEpoch;
+ private final Optional<Boolean> isClassic;
public MemberDescription(String memberId,
Optional<String> groupInstanceId,
String clientId,
String host,
MemberAssignment assignment,
- Optional<MemberAssignment> targetAssignment
- ) {
+ Optional<MemberAssignment> targetAssignment,
+ Optional<Integer> memberEpoch,
+ Optional<Boolean> isClassic) {
Review Comment:
nit: Let's keep the closing `)` on a new line as it was before.
##########
clients/src/main/java/org/apache/kafka/clients/admin/MemberDescription.java:
##########
@@ -30,21 +30,26 @@ public class MemberDescription {
private final String host;
private final MemberAssignment assignment;
private final Optional<MemberAssignment> targetAssignment;
+ private final Optional<Integer> memberEpoch;
+ private final Optional<Boolean> isClassic;
public MemberDescription(String memberId,
Review Comment:
Here, I suppose that we must introduce a new overload of the constructor
with the new arguments in order to not break compatibility of existing usages.
##########
clients/src/main/java/org/apache/kafka/clients/admin/MemberDescription.java:
##########
@@ -131,13 +140,30 @@ public Optional<MemberAssignment> targetAssignment() {
return targetAssignment;
}
+ /**
+ * The epoch of the group member.
+ */
+ public Optional<Integer> memberEpoch() {
+ return memberEpoch;
+ }
+
+ /**
+ * The flag indicating whether a member is classic.
+ */
+ public Optional<Boolean> isClassic() {
Review Comment:
Sorry for coming back on this one but I just thought about an alternative.
How about calling it `nonUpgraded`? We could set it to true when the group is a
consumer group and the member uses classic protocol. We could also only set it
when the group is a consumer group vs a classic group. This may be even more
explicit for users. What do you think? @chia7712 Do you have any thoughts too?
##########
clients/src/main/java/org/apache/kafka/clients/admin/MemberDescription.java:
##########
@@ -131,13 +140,30 @@ public Optional<MemberAssignment> targetAssignment() {
return targetAssignment;
}
+ /**
+ * The epoch of the group member.
+ */
+ public Optional<Integer> memberEpoch() {
+ return memberEpoch;
+ }
+
+ /**
+ * The flag indicating whether a member is classic.
Review Comment:
nit: How about `The flag indicating whether a member with a consumer group
uses the classic protocol. It indicates that the member was perhaps not
upgraded`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
