Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,30 @@

package org.apache.kafka.clients.admin;

import java.util.Objects;
import java.util.Optional;

import org.apache.kafka.common.ConsumerGroupState;

/**
* A listing of a consumer group in the cluster.
*/
public class ConsumerGroupListing {
private final String groupId;
private final boolean isSimpleConsumerGroup;
private final Optional<ConsumerGroupState> state;

/**
* Create an instance with the specified parameters.
*
* @param groupId Group Id
* @param isSimpleConsumerGroup If consumer group is simple or not.
* @param state The state of the consumer group
*/
public ConsumerGroupListing(String groupId, boolean isSimpleConsumerGroup) {
public ConsumerGroupListing(String groupId, boolean isSimpleConsumerGroup, Optional<ConsumerGroupState> state) {
Comment thread
mimaison marked this conversation as resolved.
this.groupId = groupId;
this.isSimpleConsumerGroup = isSimpleConsumerGroup;
this.state = state;
Comment thread
mimaison marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: usually we would write this is this.state = requireNonNull(state);

}

/**
Expand All @@ -49,11 +57,49 @@ public boolean isSimpleConsumerGroup() {
return isSimpleConsumerGroup;
}

/**
* Consumer Group state
*/
public Optional<ConsumerGroupState> state() {
return state;
}

@Override
public String toString() {
return "(" +
"groupId='" + groupId + '\'' +
", isSimpleConsumerGroup=" + isSimpleConsumerGroup +
", state=" + state +
')';
}

@Override
public int hashCode() {
return Objects.hash(groupId, isSimpleConsumerGroup, state);
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
ConsumerGroupListing other = (ConsumerGroupListing) obj;
if (groupId == null) {
if (other.groupId != null)
return false;
} else if (!groupId.equals(other.groupId))
return false;
if (isSimpleConsumerGroup != other.isSimpleConsumerGroup)
return false;
if (state == null) {
if (other.state != null)
return false;
} else if (!state.equals(other.state))
return false;
return true;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -3052,14 +3052,21 @@ void handleResponse(AbstractResponse abstractResponse) {
runnable.call(new Call("listConsumerGroups", deadline, new ConstantNodeIdProvider(node.id())) {
@Override
ListGroupsRequest.Builder createRequest(int timeoutMs) {
return new ListGroupsRequest.Builder(new ListGroupsRequestData());
List<String> states = options.states().orElse(Collections.emptySet())
.stream()
.map(s -> s.toString())
.collect(Collectors.toList());
return new ListGroupsRequest.Builder(new ListGroupsRequestData().setStates(states));
Comment thread
mimaison marked this conversation as resolved.
Outdated
}

private void maybeAddConsumerGroup(ListGroupsResponseData.ListedGroup group) {
String protocolType = group.protocolType();
if (protocolType.equals(ConsumerProtocol.PROTOCOL_TYPE) || protocolType.isEmpty()) {
final String groupId = group.groupId();
final ConsumerGroupListing groupListing = new ConsumerGroupListing(groupId, protocolType.isEmpty());
final Optional<ConsumerGroupState> state = group.groupState() == null
? Optional.empty()
: Optional.of(ConsumerGroupState.parse(group.groupState()));
final ConsumerGroupListing groupListing = new ConsumerGroupListing(groupId, protocolType.isEmpty(), state);
results.addListing(groupListing);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@

package org.apache.kafka.clients.admin;

import java.util.EnumSet;
import java.util.Optional;
import java.util.Set;

import org.apache.kafka.common.ConsumerGroupState;
import org.apache.kafka.common.annotation.InterfaceStability;

/**
Expand All @@ -26,4 +31,34 @@
*/
@InterfaceStability.Evolving
public class ListConsumerGroupsOptions extends AbstractOptions<ListConsumerGroupsOptions> {

private Optional<Set<ConsumerGroupState>> states = Optional.empty();

/**
* Only groups in these states will be returned by listConsumerGroups()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably worth adding a comment about broker compatibility with this API.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you address this comment?

* If not set, all groups are returned without their states
* throw IllegalArgumentException if states is empty
*/
public ListConsumerGroupsOptions inStates(Set<ConsumerGroupState> states) {
if (states == null || states.isEmpty()) {
throw new IllegalArgumentException("states should not be null or empty");
}
this.states = Optional.of(states);
return this;
}

/**
* All groups with their states will be returned by listConsumerGroups()
*/
public ListConsumerGroupsOptions inAnyState() {
Comment thread
mimaison marked this conversation as resolved.
Outdated
this.states = Optional.of(EnumSet.allOf(ConsumerGroupState.class));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm.. We have an UNKNOWN state in ConsumerGroupState in case the group coordinator adds a new state that the client isn't aware of. Currently we're going to pass this through the request, which is a bit odd. Furthermore, if the coordinator does add new states, we will be unable to see them using this API. I think it might be better to use a null list of states in the request to indicate that any state is needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a good point so I agree, it makes sense to return all states when null (or an empty list) is used.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This can also be argued for the state value in the response. Currently ConsumerGroupDescription stores the state as ConsumerGroupState so states the client isn't aware of are mapped to UNKNOWN so I'm doing the same in ConsumerGroupListing.

return this;
}

/**
* Returns the list of States that are requested
*/
public Optional<Set<ConsumerGroupState>> states() {
return states;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ public enum ConsumerGroupState {
this.name = name;
}


/**
* Parse a string into a consumer group state.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.kafka.common.requests;

import org.apache.kafka.common.errors.UnsupportedVersionException;
import org.apache.kafka.common.message.ListGroupsRequestData;
import org.apache.kafka.common.message.ListGroupsResponseData;
import org.apache.kafka.common.protocol.ApiKeys;
Expand Down Expand Up @@ -45,6 +46,10 @@ public Builder(ListGroupsRequestData data) {

@Override
public ListGroupsRequest build(short version) {
if (!data.states().isEmpty() && version < 4) {
throw new UnsupportedVersionException("The broker only supports ListGroups " +
"v" + version + ", but we need v4 or newer to request groups by states.");
}
return new ListGroupsRequest(data, version);
}

Expand All @@ -66,6 +71,10 @@ public ListGroupsRequest(Struct struct, short version) {
this.data = new ListGroupsRequestData(struct, version);
}

public ListGroupsRequestData data() {
return data;
}

@Override
public ListGroupsResponse getErrorResponse(int throttleTimeMs, Throwable e) {
ListGroupsResponseData listGroupsResponseData = new ListGroupsResponseData().
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@
// Version 1 and 2 are the same as version 0.
//
// Version 3 is the first flexible version.
"validVersions": "0-3",
//
// Version 4 adds the States flexible field (KIP-518).
"validVersions": "0-4",
"flexibleVersions": "3+",
"fields": [
{ "name": "States", "type": "[]string", "versions": "4+", "tag": 0, "taggedVersions": "4+",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I missed this from the discussion, but why are we bumping the version if we are only adding tagged fields? Is it so that we can detect whether the capability is supported? If so, then I wonder why we don't make this a regular field.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes the version bump is necessary to detect if this field was supported.

As we're bumping the version and as you said in #8238 (comment) the overhead of the extra field on this API is not a concern, it's probably simpler to use a regular field.

"about": "The states of the groups we want to list. If empty, all groups are returned, without state.", "fields": [
{ "name": "Name", "type": "string", "versions": "4+", "about": "The group state" }
]}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
// Starting in version 2, on quota violation, brokers send out responses before throttling.
//
// Version 3 is the first flexible version.
"validVersions": "0-3",
//
// Version 4 adds the GroupState flexible field (KIP-518).
"validVersions": "0-4",
"flexibleVersions": "3+",
"fields": [
{ "name": "ThrottleTimeMs", "type": "int32", "versions": "1+", "ignorable": true,
Expand All @@ -34,7 +36,9 @@
{ "name": "GroupId", "type": "string", "versions": "0+", "entityType": "groupId",
"about": "The group ID." },
{ "name": "ProtocolType", "type": "string", "versions": "0+",
"about": "The group protocol type." }
"about": "The group protocol type." },
{ "name": "GroupState", "type": "string", "versions": "4+", "tag": 0, "taggedVersions": "4+", "ignorable": true,
"nullableVersions": "4+", "default": "null", "about": "The group state string." }
]}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.kafka.clients.consumer.OffsetAndMetadata;
import org.apache.kafka.clients.consumer.internals.ConsumerProtocol;
import org.apache.kafka.common.Cluster;
import org.apache.kafka.common.ConsumerGroupState;
import org.apache.kafka.common.ElectionType;
import org.apache.kafka.common.KafkaException;
import org.apache.kafka.common.KafkaFuture;
Expand Down Expand Up @@ -1282,6 +1283,7 @@ public void testListConsumerGroups() throws Exception {
Set<String> groupIds = new HashSet<>();
for (ConsumerGroupListing listing : listings) {
groupIds.add(listing.groupId());
assertFalse(listing.state().isPresent());
}

assertEquals(Utils.mkSet("group-1", "group-2", "group-3"), groupIds);
Expand Down Expand Up @@ -1312,6 +1314,39 @@ public void testListConsumerGroupsMetadataFailure() throws Exception {
}
}

@Test
public void testListConsumerGroupsWithStates() throws Exception {
Comment thread
mimaison marked this conversation as resolved.
Outdated
try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(mockCluster(1, 0))) {
env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());

env.kafkaClient().prepareResponse(prepareMetadataResponse(env.cluster(), Errors.NONE));

env.kafkaClient().prepareResponseFrom(
new ListGroupsResponse(new ListGroupsResponseData()
.setErrorCode(Errors.NONE.code())
.setGroups(Arrays.asList(
new ListGroupsResponseData.ListedGroup()
.setGroupId("group-1")
.setProtocolType(ConsumerProtocol.PROTOCOL_TYPE)
.setGroupState("Stable"),
new ListGroupsResponseData.ListedGroup()
.setGroupId("group-2")
.setGroupState("Empty")))),
env.cluster().nodeById(0));

final ListConsumerGroupsOptions options = new ListConsumerGroupsOptions().inAnyState();
final ListConsumerGroupsResult result = env.adminClient().listConsumerGroups(options);
Collection<ConsumerGroupListing> listings = result.valid().get();

assertEquals(2, listings.size());
List<ConsumerGroupListing> expected = new ArrayList<>();
expected.add(new ConsumerGroupListing("group-2", true, Optional.of(ConsumerGroupState.EMPTY)));
expected.add(new ConsumerGroupListing("group-1", false, Optional.of(ConsumerGroupState.STABLE)));
assertEquals(expected, listings);
assertEquals(0, result.errors().get().size());
}
}

@Test
public void testOffsetCommitNumRetries() throws Exception {
final Cluster cluster = mockCluster(3, 0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.kafka.common.requests;

import org.apache.kafka.common.ConsumerGroupState;
import org.apache.kafka.common.ElectionType;
import org.apache.kafka.common.IsolationLevel;
import org.apache.kafka.common.Node;
Expand Down Expand Up @@ -229,8 +230,10 @@ public void testSerialization() throws Exception {
checkRequest(createLeaveGroupRequest(), true);
checkErrorResponse(createLeaveGroupRequest(), new UnknownServerException(), true);
checkResponse(createLeaveGroupResponse(), 0, true);
checkRequest(createListGroupsRequest(), true);
checkErrorResponse(createListGroupsRequest(), new UnknownServerException(), true);
for (short v = ApiKeys.LIST_GROUPS.oldestVersion(); v <= ApiKeys.LIST_GROUPS.latestVersion(); v++) {
checkRequest(createListGroupsRequest(v), false);
checkErrorResponse(createListGroupsRequest(v), new UnknownServerException(), true);
}
checkResponse(createListGroupsResponse(), 0, true);
checkRequest(createDescribeGroupRequest(), true);
checkErrorResponse(createDescribeGroupRequest(), new UnknownServerException(), true);
Expand Down Expand Up @@ -810,6 +813,13 @@ public void testValidApiVersionsRequest() {
assertTrue(request.isValid());
}

@Test(expected = UnsupportedVersionException.class)
public void testListGroupRequestV3FailsWithStates() {
ListGroupsRequestData data = new ListGroupsRequestData()
.setStates(asList(ConsumerGroupState.STABLE.name()));
new ListGroupsRequest.Builder(data).build((short) 3);
}

@Test
public void testInvalidApiVersionsRequest() {
testInvalidCase("java@apache_kafka", "0.0.0-SNAPSHOT");
Expand Down Expand Up @@ -1066,8 +1076,11 @@ private SyncGroupResponse createSyncGroupResponse(int version) {
return new SyncGroupResponse(data);
}

private ListGroupsRequest createListGroupsRequest() {
return new ListGroupsRequest.Builder(new ListGroupsRequestData()).build();
private ListGroupsRequest createListGroupsRequest(short version) {
ListGroupsRequestData data = new ListGroupsRequestData();
if (version >= 4)
data.setStates(Arrays.asList("Stable"));
return new ListGroupsRequest.Builder(data).build(version);
}

private ListGroupsResponse createListGroupsResponse() {
Expand Down Expand Up @@ -1133,7 +1146,6 @@ private DeleteGroupsResponse createDeleteGroupsResponse() {
);
}

@SuppressWarnings("deprecation")
private ListOffsetRequest createListOffsetRequest(int version) {
if (version == 0) {
Map<TopicPartition, ListOffsetRequest.PartitionData> offsetData = Collections.singletonMap(
Expand Down Expand Up @@ -1164,7 +1176,6 @@ private ListOffsetRequest createListOffsetRequest(int version) {
}
}

@SuppressWarnings("deprecation")
private ListOffsetResponse createListOffsetResponse(int version) {
if (version == 0) {
Map<TopicPartition, ListOffsetResponse.PartitionData> responseData = new HashMap<>();
Expand Down
Loading