-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-13888: Addition of Information in DescribeQuorumResponse about Voter Lag #12206
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
Changes from 13 commits
efb9c20
b85a5d8
1b1c022
d517ac9
805d685
cfde869
985a8ad
ca26c56
07bcc41
668b368
73f3b31
b4cc945
51f7dd2
868b567
9d1d90f
739b400
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.clients.admin; | ||
|
|
||
| /** | ||
| * Options for {@link Admin#describeMetadataQuorum(DescribeMetadataQuorumOptions)} | ||
| */ | ||
| public class DescribeMetadataQuorumOptions extends AbstractOptions<DescribeMetadataQuorumOptions> { | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.clients.admin; | ||
|
|
||
| import org.apache.kafka.common.KafkaFuture; | ||
|
|
||
| /** | ||
| * The result of {@link Admin#describeMetadataQuorum(DescribeMetadataQuorumOptions)} | ||
| */ | ||
| public class DescribeMetadataQuorumResult { | ||
|
|
||
| private final KafkaFuture<QuorumInfo> quorumInfo; | ||
|
|
||
| DescribeMetadataQuorumResult(KafkaFuture<QuorumInfo> quorumInfo) { | ||
| this.quorumInfo = quorumInfo; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a future containing the QuorumInfo | ||
| */ | ||
| public KafkaFuture<QuorumInfo> quorumInfo() { | ||
| return quorumInfo; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,6 +135,7 @@ | |
| import org.apache.kafka.common.message.DescribeLogDirsRequestData; | ||
| import org.apache.kafka.common.message.DescribeLogDirsRequestData.DescribableLogDirTopic; | ||
| import org.apache.kafka.common.message.DescribeLogDirsResponseData; | ||
| import org.apache.kafka.common.message.DescribeQuorumResponseData; | ||
| import org.apache.kafka.common.message.DescribeUserScramCredentialsRequestData; | ||
| import org.apache.kafka.common.message.DescribeUserScramCredentialsRequestData.UserName; | ||
| import org.apache.kafka.common.message.DescribeUserScramCredentialsResponseData; | ||
|
|
@@ -208,6 +209,9 @@ | |
| import org.apache.kafka.common.requests.DescribeLogDirsResponse; | ||
| import org.apache.kafka.common.requests.DescribeUserScramCredentialsRequest; | ||
| import org.apache.kafka.common.requests.DescribeUserScramCredentialsResponse; | ||
| import org.apache.kafka.common.requests.DescribeQuorumRequest; | ||
| import org.apache.kafka.common.requests.DescribeQuorumRequest.Builder; | ||
| import org.apache.kafka.common.requests.DescribeQuorumResponse; | ||
| import org.apache.kafka.common.requests.ElectLeadersRequest; | ||
| import org.apache.kafka.common.requests.ElectLeadersResponse; | ||
| import org.apache.kafka.common.requests.ExpireDelegationTokenRequest; | ||
|
|
@@ -257,6 +261,7 @@ | |
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
| import java.util.OptionalLong; | ||
| import java.util.Set; | ||
| import java.util.TreeMap; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
@@ -268,6 +273,8 @@ | |
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import static org.apache.kafka.common.internals.Topic.METADATA_TOPIC_NAME; | ||
| import static org.apache.kafka.common.internals.Topic.METADATA_TOPIC_PARTITION; | ||
| import static org.apache.kafka.common.message.AlterPartitionReassignmentsRequestData.ReassignablePartition; | ||
| import static org.apache.kafka.common.message.AlterPartitionReassignmentsResponseData.ReassignablePartitionResponse; | ||
| import static org.apache.kafka.common.message.AlterPartitionReassignmentsResponseData.ReassignableTopicResponse; | ||
|
|
@@ -4321,6 +4328,88 @@ void handleFailure(Throwable throwable) { | |
| return new UpdateFeaturesResult(new HashMap<>(updateFutures)); | ||
| } | ||
|
|
||
| @Override | ||
| public DescribeMetadataQuorumResult describeMetadataQuorum(DescribeMetadataQuorumOptions options) { | ||
| NodeProvider provider = new LeastLoadedNodeProvider(); | ||
|
|
||
| final KafkaFutureImpl<QuorumInfo> future = new KafkaFutureImpl<>(); | ||
| final long now = time.milliseconds(); | ||
| final Call call = new Call( | ||
| "describeMetadataQuorum", calcDeadlineMs(now, options.timeoutMs()), provider) { | ||
|
|
||
| private QuorumInfo createQuorumResult(final DescribeQuorumResponseData.PartitionData partition) { | ||
| List<QuorumInfo.ReplicaState> voters = new ArrayList<>(); | ||
| List<QuorumInfo.ReplicaState> observers = new ArrayList<>(); | ||
| partition.currentVoters().forEach(v -> { | ||
| voters.add(new QuorumInfo.ReplicaState(v.replicaId(), | ||
| v.logEndOffset(), | ||
| v.lastFetchTimestamp() == -1 ? OptionalLong.empty() : OptionalLong.of(v.lastFetchTimestamp()), | ||
| v.lastCaughtUpTimestamp() == -1 ? OptionalLong.empty() : OptionalLong.of(v.lastCaughtUpTimestamp()))); | ||
| }); | ||
| partition.observers().forEach(o -> { | ||
| observers.add(new QuorumInfo.ReplicaState(o.replicaId(), | ||
| o.logEndOffset(), | ||
| o.lastFetchTimestamp() == -1 ? OptionalLong.empty() : OptionalLong.of(o.lastFetchTimestamp()), | ||
| o.lastCaughtUpTimestamp() == -1 ? OptionalLong.empty() : OptionalLong.of(o.lastCaughtUpTimestamp()))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we extract this bloc into an helper method and share it with the voters part?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really think this is worth another method.?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. We should avoid code duplication. Btw, you could also use the stream api here.: |
||
| }); | ||
| QuorumInfo info = new QuorumInfo(partition.leaderId(), voters, observers); | ||
| return info; | ||
| } | ||
|
|
||
| @Override | ||
| DescribeQuorumRequest.Builder createRequest(int timeoutMs) { | ||
|
hachikuji marked this conversation as resolved.
Outdated
|
||
| return new Builder(DescribeQuorumRequest.singletonRequest( | ||
| new TopicPartition(METADATA_TOPIC_NAME, METADATA_TOPIC_PARTITION.partition()))); | ||
| } | ||
|
|
||
| @Override | ||
| void handleResponse(AbstractResponse response) { | ||
| final DescribeQuorumResponse quorumResponse = (DescribeQuorumResponse) response; | ||
| if (quorumResponse.data().errorCode() != Errors.NONE.code()) { | ||
| throw Errors.forCode(quorumResponse.data().errorCode()).exception(); | ||
|
dajac marked this conversation as resolved.
Outdated
|
||
| } | ||
| if (quorumResponse.data().topics().size() != 1) { | ||
| String msg = String.format("DescribeMetadataQuorum received %d topics when 1 was expected", | ||
| quorumResponse.data().topics().size()); | ||
| log.debug(msg); | ||
| throw new UnknownServerException(msg); | ||
| } | ||
| DescribeQuorumResponseData.TopicData topic = quorumResponse.data().topics().get(0); | ||
| if (!topic.topicName().equals(METADATA_TOPIC_NAME)) { | ||
| String msg = String.format("DescribeMetadataQuorum received a topic with name %s when %s was expected", | ||
| topic.topicName(), METADATA_TOPIC_NAME); | ||
| log.debug(msg); | ||
| throw new UnknownServerException(msg); | ||
| } | ||
| if (topic.partitions().size() != 1) { | ||
| String msg = String.format("DescribeMetadataQuorum received a topic %s with %d partitions when 1 was expected", | ||
| topic.topicName(), topic.partitions().size()); | ||
| log.debug(msg); | ||
| throw new UnknownServerException(msg); | ||
| } | ||
| DescribeQuorumResponseData.PartitionData partition = topic.partitions().get(0); | ||
| if (partition.partitionIndex() != METADATA_TOPIC_PARTITION.partition()) { | ||
| String msg = String.format("DescribeMetadataQuorum received a single partition with index %d when %d was expected", | ||
| partition.partitionIndex(), METADATA_TOPIC_PARTITION.partition()); | ||
| log.debug(msg); | ||
| throw new UnknownServerException(msg); | ||
| } | ||
| if (partition.errorCode() != Errors.NONE.code()) { | ||
| throw Errors.forCode(partition.errorCode()).exception(); | ||
| } | ||
| future.complete(createQuorumResult(partition)); | ||
| } | ||
|
|
||
| @Override | ||
| void handleFailure(Throwable throwable) { | ||
| future.completeExceptionally(throwable); | ||
| } | ||
| }; | ||
|
|
||
| runnable.call(call, now); | ||
| return new DescribeMetadataQuorumResult(future); | ||
| } | ||
|
|
||
| @Override | ||
| public UnregisterBrokerResult unregisterBroker(int brokerId, UnregisterBrokerOptions options) { | ||
| final KafkaFutureImpl<Void> future = new KafkaFutureImpl<>(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.clients.admin; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.OptionalLong; | ||
|
|
||
| /** | ||
| * This is used to describe per-partition state in the DescribeQuorumResponse. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reference to DescribeQuorumResponse is a bit weird here for our users. Could we to better describe what QuorumInfo represents?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree with this. Maybe we can just say that this class contains useful debugging state for KRaft replication.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment may have been missed. |
||
| */ | ||
| public class QuorumInfo { | ||
| private final Integer leaderId; | ||
| private final List<ReplicaState> voters; | ||
| private final List<ReplicaState> observers; | ||
|
|
||
| QuorumInfo(Integer leaderId, List<ReplicaState> voters, List<ReplicaState> observers) { | ||
| this.leaderId = leaderId; | ||
| this.voters = voters; | ||
| this.observers = observers; | ||
| } | ||
|
|
||
| public Integer leaderId() { | ||
| return leaderId; | ||
| } | ||
|
|
||
| public List<ReplicaState> voters() { | ||
| return voters; | ||
| } | ||
|
|
||
| public List<ReplicaState> observers() { | ||
| return observers; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| QuorumInfo that = (QuorumInfo) o; | ||
| return leaderId.equals(that.leaderId) | ||
| && voters.equals(that.voters) | ||
| && observers.equals(that.observers); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(leaderId, voters, observers); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "QuorumInfo(" + | ||
| "leaderId=" + leaderId + | ||
| ", voters=" + voters.toString() + | ||
| ", observers=" + observers.toString() + | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I suppose that we could remove the |
||
| ')'; | ||
| } | ||
|
|
||
| public static class ReplicaState { | ||
| private final int replicaId; | ||
| private final long logEndOffset; | ||
| private final OptionalLong lastFetchTimeMs; | ||
| private final OptionalLong lastCaughtUpTimeMs; | ||
|
|
||
| ReplicaState() { | ||
| this(0, 0, OptionalLong.empty(), OptionalLong.empty()); | ||
| } | ||
|
|
||
| ReplicaState(int replicaId, long logEndOffset, | ||
| OptionalLong lastFetchTimeMs, OptionalLong lastCaughtUpTimeMs) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The code format seems a bit off here. I think that we would format like this: |
||
| this.replicaId = replicaId; | ||
| this.logEndOffset = logEndOffset; | ||
| this.lastFetchTimeMs = lastFetchTimeMs; | ||
| this.lastCaughtUpTimeMs = lastCaughtUpTimeMs; | ||
| } | ||
|
|
||
| /** | ||
| * Return the ID for this replica. | ||
| * @return The ID for this replica | ||
| */ | ||
| public int replicaId() { | ||
|
dajac marked this conversation as resolved.
Outdated
|
||
| return replicaId; | ||
| } | ||
|
|
||
| /** | ||
| * Return the logEndOffset known by the leader for this replica. | ||
| * @return The logEndOffset for this replica | ||
| */ | ||
| public long logEndOffset() { | ||
| return logEndOffset; | ||
| } | ||
|
|
||
| /** | ||
| * Return the lastFetchTime in milliseconds for this replica. | ||
| * @return The value of the lastFetchTime if known, empty otherwise | ||
| */ | ||
| public OptionalLong lastFetchTimeMs() { | ||
| return lastFetchTimeMs; | ||
| } | ||
|
|
||
| /** | ||
| * Return the lastCaughtUpTime in milliseconds for this replica. | ||
| * @return The value of the lastCaughtUpTime if known, empty otherwise | ||
| */ | ||
| public OptionalLong lastCaughtUpTimeMs() { | ||
| return lastCaughtUpTimeMs; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| ReplicaState that = (ReplicaState) o; | ||
| return replicaId == that.replicaId | ||
| && logEndOffset == that.logEndOffset | ||
| && lastFetchTimeMs.equals(that.lastFetchTimeMs) | ||
| && lastCaughtUpTimeMs.equals(that.lastCaughtUpTimeMs); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(replicaId, logEndOffset, lastFetchTimeMs, lastCaughtUpTimeMs); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "ReplicaState(" + | ||
| "replicaId=" + replicaId + | ||
| ", logEndOffset=" + logEndOffset + | ||
| ", lastFetchTimeMs=" + lastFetchTimeMs + | ||
| ", lastCaughtUpTimeMs=" + lastCaughtUpTimeMs + | ||
| ')'; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,4 +93,5 @@ public static DescribeQuorumResponseData singletonResponse(TopicPartition topicP | |
| public static DescribeQuorumResponse parse(ByteBuffer buffer, short version) { | ||
| return new DescribeQuorumResponse(new DescribeQuorumResponseData(new ByteBufferAccessor(buffer), version)); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This empty line could be removed. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very common for API details to change during implementation. Once we're satisfied and ready to merge, we should update the KIP. Typically we would also send a message to the vote thread in case there are any concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the KIP and the thread once this PR is approved to reflect the changes here.