Skip to content

MINOR; Clean up LeaderAndIsrResponse construction in ReplicaManager#becomeLeaderOrFollower #10234

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.

Sign up for GitHub

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

Merged
dajac merged 4 commits into from
Mar 4, 2021
Merged

MINOR; Clean up LeaderAndIsrResponse construction in ReplicaManager#becomeLeaderOrFollower #10234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1953ab2
MINOR; Clean up LeaderAndIsrResponse construction in `ReplicaManager#…
dajac Mar 1, 2021
6872269
Address comment
dajac Mar 3, 2021
1eda1b3
Fix LeaderAndIsrRequestTest#testGetErrorResponse to really test all t…
dajac Mar 3, 2021
fe54ad5
fixup
dajac Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions clients/src/main/java/org/apache/kafka/common/requests/LeaderAndIsrRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,24 +145,21 @@ public LeaderAndIsrResponse getErrorResponse(int throttleTimeMs, Throwable e) {
.setErrorCode(error.code()));
}
responseData.setPartitionErrors(partitions);
return new LeaderAndIsrResponse(responseData, version());
}

List<LeaderAndIsrTopicError> topics = new ArrayList<>(data.topicStates().size());
Map<String, Uuid> topicIds = topicIds();
for (LeaderAndIsrTopicState topicState : data.topicStates()) {
LeaderAndIsrTopicError topicError = new LeaderAndIsrTopicError();
topicError.setTopicId(topicIds.get(topicState.topicName()));
List<LeaderAndIsrPartitionError> partitions = new ArrayList<>(topicState.partitionStates().size());
for (LeaderAndIsrPartitionState partition : topicState.partitionStates()) {
partitions.add(new LeaderAndIsrPartitionError()
} else {
for (LeaderAndIsrTopicState topicState : data.topicStates()) {
List<LeaderAndIsrPartitionError> partitions = new ArrayList<>(
topicState.partitionStates().size());
for (LeaderAndIsrPartitionState partition : topicState.partitionStates()) {
partitions.add(new LeaderAndIsrPartitionError()
.setPartitionIndex(partition.partitionIndex())
Comment thread
dajac marked this conversation as resolved.
.setErrorCode(error.code()));
}
responseData.topics().add(new LeaderAndIsrTopicError()
.setTopicId(topicState.topicId())
.setPartitionErrors(partitions));
}
topicError.setPartitionErrors(partitions);
topics.add(topicError);
}
responseData.setTopics(topics);

return new LeaderAndIsrResponse(responseData, version());
}

Expand Down
6 changes: 3 additions & 3 deletions clients/src/main/java/org/apache/kafka/common/requests/LeaderAndIsrResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@
import org.apache.kafka.common.Uuid;
import org.apache.kafka.common.message.LeaderAndIsrResponseData;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrTopicError;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrTopicErrorCollection;
import org.apache.kafka.common.protocol.ApiKeys;
import org.apache.kafka.common.protocol.ByteBufferAccessor;
import org.apache.kafka.common.protocol.Errors;

import java.nio.ByteBuffer;
import java.util.Collections;
import java.util.List;
import java.util.HashMap;
import java.util.Map;

Expand All @@ -39,15 +39,15 @@ public class LeaderAndIsrResponse extends AbstractResponse {
* STALE_BROKER_EPOCH (77)
*/
private final LeaderAndIsrResponseData data;
private short version;
private final short version;

public LeaderAndIsrResponse(LeaderAndIsrResponseData data, short version) {
super(ApiKeys.LEADER_AND_ISR);
this.data = data;
this.version = version;
}

public List<LeaderAndIsrTopicError> topics() {
public LeaderAndIsrTopicErrorCollection topics() {
return this.data.topics();
}

Expand Down
3 changes: 2 additions & 1 deletion clients/src/main/resources/common/message/LeaderAndIsrResponse.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"about": "Each partition in v0 to v4 message."},
{ "name": "Topics", "type": "[]LeaderAndIsrTopicError", "versions": "5+",
"about": "Each topic", "fields": [
{ "name": "TopicId", "type": "uuid", "versions": "5+", "about": "The unique topic ID" },
{ "name": "TopicId", "type": "uuid", "versions": "5+", "mapKey": true,
"about": "The unique topic ID" },
{ "name": "PartitionErrors", "type": "[]LeaderAndIsrPartitionError", "versions": "5+",
"about": "Each partition."}
]}
Expand Down
41 changes: 33 additions & 8 deletions clients/src/test/java/org/apache/kafka/common/requests/LeaderAndIsrRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import org.apache.kafka.common.message.LeaderAndIsrRequestData;
import org.apache.kafka.common.message.LeaderAndIsrRequestData.LeaderAndIsrLiveLeader;
import org.apache.kafka.common.message.LeaderAndIsrRequestData.LeaderAndIsrPartitionState;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrPartitionError;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrTopicError;
import org.apache.kafka.common.protocol.ByteBufferAccessor;
import org.apache.kafka.common.protocol.Errors;
import org.apache.kafka.test.TestUtils;
Expand Down Expand Up @@ -59,18 +61,41 @@ public void testUnsupportedVersion() {

@Test
public void testGetErrorResponse() {
Uuid id = Uuid.randomUuid();
for (short version = LEADER_AND_ISR.oldestVersion(); version < LEADER_AND_ISR.latestVersion(); version++) {
LeaderAndIsrRequest.Builder builder = new LeaderAndIsrRequest.Builder(version, 0, 0, 0,
Collections.emptyList(), Collections.singletonMap("topic", id), Collections.emptySet());
LeaderAndIsrRequest request = builder.build();
Uuid topicId = Uuid.randomUuid();
String topicName = "topic";
int partition = 0;

for (short version = LEADER_AND_ISR.oldestVersion(); version <= LEADER_AND_ISR.latestVersion(); version++) {
LeaderAndIsrRequest request = new LeaderAndIsrRequest.Builder(version, 0, 0, 0,
Collections.singletonList(new LeaderAndIsrPartitionState()
.setTopicName(topicName)
.setPartitionIndex(partition)),
Collections.singletonMap(topicName, topicId),
Collections.emptySet()
).build(version);

LeaderAndIsrResponse response = request.getErrorResponse(0,
new ClusterAuthorizationException("Not authorized"));
new ClusterAuthorizationException("Not authorized"));

assertEquals(Errors.CLUSTER_AUTHORIZATION_FAILED, response.error());

if (version < 5) {
assertEquals(0, response.topics().size());
assertEquals(
Collections.singletonList(new LeaderAndIsrPartitionError()
.setTopicName(topicName)
.setPartitionIndex(partition)
.setErrorCode(Errors.CLUSTER_AUTHORIZATION_FAILED.code())),
response.data().partitionErrors());
assertEquals(0, response.data().topics().size());
} else {
assertEquals(id, response.topics().get(0).topicId());
LeaderAndIsrTopicError topicState = response.topics().find(topicId);
assertEquals(topicId, topicState.topicId());
assertEquals(
Collections.singletonList(new LeaderAndIsrPartitionError()
.setPartitionIndex(partition)
.setErrorCode(Errors.CLUSTER_AUTHORIZATION_FAILED.code())),
topicState.partitionErrors());
assertEquals(0, response.data().partitionErrors().size());
}
}
}
Expand Down
11 changes: 6 additions & 5 deletions clients/src/test/java/org/apache/kafka/common/requests/LeaderAndIsrResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.apache.kafka.common.message.LeaderAndIsrResponseData;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrTopicError;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrPartitionError;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrTopicErrorCollection;
import org.apache.kafka.common.protocol.ApiKeys;
import org.apache.kafka.common.protocol.Errors;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -80,7 +81,7 @@ public void testErrorCountsWithTopLevelError() {
.setPartitionErrors(partitions), version);
} else {
Uuid id = Uuid.randomUuid();
List<LeaderAndIsrTopicError> topics = createTopic(id, asList(Errors.NONE, Errors.NOT_LEADER_OR_FOLLOWER));
LeaderAndIsrTopicErrorCollection topics = createTopic(id, asList(Errors.NONE, Errors.NOT_LEADER_OR_FOLLOWER));
response = new LeaderAndIsrResponse(new LeaderAndIsrResponseData()
.setErrorCode(Errors.UNKNOWN_SERVER_ERROR.code())
.setTopics(topics), version);
Expand All @@ -101,7 +102,7 @@ public void testErrorCountsNoTopLevelError() {
.setPartitionErrors(partitions), version);
} else {
Uuid id = Uuid.randomUuid();
List<LeaderAndIsrTopicError> topics = createTopic(id, asList(Errors.NONE, Errors.CLUSTER_AUTHORIZATION_FAILED));
LeaderAndIsrTopicErrorCollection topics = createTopic(id, asList(Errors.NONE, Errors.CLUSTER_AUTHORIZATION_FAILED));
response = new LeaderAndIsrResponse(new LeaderAndIsrResponseData()
.setErrorCode(Errors.NONE.code())
.setTopics(topics), version);
Expand Down Expand Up @@ -130,7 +131,7 @@ public void testToString() {

} else {
Uuid id = Uuid.randomUuid();
List<LeaderAndIsrTopicError> topics = createTopic(id, asList(Errors.NONE, Errors.CLUSTER_AUTHORIZATION_FAILED));
LeaderAndIsrTopicErrorCollection topics = createTopic(id, asList(Errors.NONE, Errors.CLUSTER_AUTHORIZATION_FAILED));
response = new LeaderAndIsrResponse(new LeaderAndIsrResponseData()
.setErrorCode(Errors.NONE.code())
.setTopics(topics), version);
Expand All @@ -155,8 +156,8 @@ private List<LeaderAndIsrPartitionError> createPartitions(String topicName, List
return partitions;
}

private List<LeaderAndIsrTopicError> createTopic(Uuid id, List<Errors> errors) {
List<LeaderAndIsrTopicError> topics = new ArrayList<>();
private LeaderAndIsrTopicErrorCollection createTopic(Uuid id, List<Errors> errors) {
LeaderAndIsrTopicErrorCollection topics = new LeaderAndIsrTopicErrorCollection();
LeaderAndIsrTopicError topic = new LeaderAndIsrTopicError();
topic.setTopicId(id);
List<LeaderAndIsrPartitionError> partitions = new ArrayList<>();
Expand Down
3 changes: 2 additions & 1 deletion clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
import org.apache.kafka.common.message.JoinGroupResponseData.JoinGroupResponseMember;
import org.apache.kafka.common.message.LeaderAndIsrRequestData.LeaderAndIsrPartitionState;
import org.apache.kafka.common.message.LeaderAndIsrResponseData;
import org.apache.kafka.common.message.LeaderAndIsrResponseData.LeaderAndIsrTopicErrorCollection;
import org.apache.kafka.common.message.LeaveGroupRequestData.MemberIdentity;
import org.apache.kafka.common.message.LeaveGroupResponseData;
import org.apache.kafka.common.message.ListGroupsRequestData;
Expand Down Expand Up @@ -1674,7 +1675,7 @@ private LeaderAndIsrResponse createLeaderAndIsrResponse(int version) {
new LeaderAndIsrResponseData.LeaderAndIsrPartitionError()
.setPartitionIndex(0)
.setErrorCode(Errors.NONE.code()));
List<LeaderAndIsrResponseData.LeaderAndIsrTopicError> topics = new ArrayList<>();
LeaderAndIsrTopicErrorCollection topics = new LeaderAndIsrTopicErrorCollection();
topics.add(new LeaderAndIsrResponseData.LeaderAndIsrTopicError()
.setTopicId(Uuid.randomUuid())
.setPartitionErrors(partition));
Expand Down
43 changes: 17 additions & 26 deletions core/src/main/scala/kafka/server/ReplicaManager.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,38 +1441,29 @@ class ReplicaManager(val config: KafkaConfig,
replicaFetcherManager.shutdownIdleFetcherThreads()
replicaAlterLogDirsManager.shutdownIdleFetcherThreads()
onLeadershipChange(partitionsBecomeLeader, partitionsBecomeFollower)
if (leaderAndIsrRequest.version() < 5) {
val responsePartitions = responseMap.iterator.map { case (tp, error) =>
new LeaderAndIsrPartitionError()

val data = new LeaderAndIsrResponseData().setErrorCode(Errors.NONE.code)
if (leaderAndIsrRequest.version < 5) {
responseMap.forKeyValue { (tp, error) =>
data.partitionErrors.add(new LeaderAndIsrPartitionError()
.setTopicName(tp.topic)
.setPartitionIndex(tp.partition)
.setErrorCode(error.code)
}.toBuffer
new LeaderAndIsrResponse(new LeaderAndIsrResponseData()
.setErrorCode(Errors.NONE.code)
.setPartitionErrors(responsePartitions.asJava), leaderAndIsrRequest.version())
.setErrorCode(error.code))
}
} else {
val topics = new mutable.HashMap[String, List[LeaderAndIsrPartitionError]]
responseMap.asJava.forEach { case (tp, error) =>
if (!topics.contains(tp.topic)) {
topics.put(tp.topic, List(new LeaderAndIsrPartitionError()
.setPartitionIndex(tp.partition)
.setErrorCode(error.code)))
} else {
topics.put(tp.topic, new LeaderAndIsrPartitionError()
.setPartitionIndex(tp.partition)
.setErrorCode(error.code)::topics(tp.topic))
responseMap.forKeyValue { (tp, error) =>
val topicId = topicIds.get(tp.topic)
var topic = data.topics.find(topicId)
if (topic == null) {
topic = new LeaderAndIsrTopicError().setTopicId(topicId)
data.topics.add(topic)
}
topic.partitionErrors.add(new LeaderAndIsrPartitionError()
.setPartitionIndex(tp.partition)
Comment thread
dajac marked this conversation as resolved.
.setErrorCode(error.code))
}
val topicErrors = topics.iterator.map { case (topic, partitionError) =>
new LeaderAndIsrTopicError()
.setTopicId(topicIds.get(topic))
.setPartitionErrors(partitionError.asJava)
}.toBuffer
new LeaderAndIsrResponse(new LeaderAndIsrResponseData()
.setErrorCode(Errors.NONE.code)
.setTopics(topicErrors.asJava), leaderAndIsrRequest.version())
}
new LeaderAndIsrResponse(data, leaderAndIsrRequest.version)
}
}
val endMs = time.milliseconds()
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/scala/unit/kafka/controller/ControllerChannelManagerTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,17 +847,17 @@ class ControllerChannelManagerTest {
sentRequests.filter(_.request.apiKey == ApiKeys.LEADER_AND_ISR).filter(_.responseCallback != null).foreach { sentRequest =>
val leaderAndIsrRequest = sentRequest.request.build().asInstanceOf[LeaderAndIsrRequest]
val topicIds = leaderAndIsrRequest.topicIds
val topicErrors = leaderAndIsrRequest.data.topicStates.asScala.map(t =>
new LeaderAndIsrTopicError()
val data = new LeaderAndIsrResponseData()
.setErrorCode(error.code)
leaderAndIsrRequest.data.topicStates.asScala.foreach { t =>
data.topics.add(new LeaderAndIsrTopicError()
.setTopicId(topicIds.get(t.topicName))
.setPartitionErrors(t.partitionStates.asScala.map(p =>
new LeaderAndIsrPartitionError()
.setPartitionIndex(p.partitionIndex)
.setErrorCode(error.code)).asJava))
val leaderAndIsrResponse = new LeaderAndIsrResponse(
new LeaderAndIsrResponseData()
.setErrorCode(error.code)
.setTopics(topicErrors.toBuffer.asJava), leaderAndIsrRequest.version())
}
val leaderAndIsrResponse = new LeaderAndIsrResponse(data, leaderAndIsrRequest.version)
sentRequest.responseCallback(leaderAndIsrResponse)
}
}
Expand Down