MINOR: A few cleanups for DescribeQuorum APIs #12548

hachikuji · 2022-08-22T23:44:54Z

A few small cleanups:

Change field types in QuorumInfo:
- leaderId: Integer -> int
- leaderEpoch: Integer -> long (to allow for type expansion in the future)
- highWatermark: Long -> long
Use field names lastFetchTimestamp and lastCaughtUpTimestamp consistently
Move construction of DescribeQuorumResponseData.PartitionData into LeaderState
Consolidate fetch time/offset update logic into LeaderState.ReplicaState.updateFollowerState

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

jsancio

Thanks for the changes. Wanted to give you a quick review. Working on a more thorough review.

clients/src/main/resources/common/message/DescribeQuorumResponse.json

showuon

Thanks for the PR. Left some comments.

showuon · 2022-08-23T03:30:20Z

clients/src/main/java/org/apache/kafka/clients/admin/QuorumInfo.java

-    private final Integer leaderEpoch;
-    private final Long highWatermark;
+    private final int leaderId;
+    private final long leaderEpoch;


The leaderEpoch is in int32 type, any reason we change to use long here?

kafka/clients/src/main/resources/common/message/DescribeQuorumResponse.json

Lines 37 to 38 in add4ca6

{ "name": "LeaderEpoch", "type": "int32", "versions": "0+",

"about": "The latest known leader epoch"},

showuon · 2022-08-23T03:32:29Z

clients/src/main/java/org/apache/kafka/clients/admin/QuorumInfo.java

-        return leaderId.equals(that.leaderId)
-            && voters.equals(that.voters)
-            && observers.equals(that.observers);
+        return leaderId == that.leaderId
+            && leaderEpoch == that.leaderEpoch
+            && highWatermark == that.highWatermark
+            && Objects.equals(highWatermarkUpdateTimeMs, that.highWatermarkUpdateTimeMs)
+            && Objects.equals(voters, that.voters)
+            && Objects.equals(observers, that.observers);


nice catch!

showuon · 2022-08-23T06:21:50Z

core/src/main/scala/kafka/admin/MetadataQuorumCommand.scala

+          |LeaderId:                   ${quorumInfo.leaderId}
+          |LeaderEpoch:                ${quorumInfo.leaderEpoch}
+          |HighWatermark:              ${quorumInfo.highWatermark}
+          |HighWatermarkUpdateTimeMs:  ${quorumInfo.highWatermarkUpdateTimeMs}


Since this is a script output, I think it'd better we did some pre-processing to avoid printing out something like: OptionalLong[12345] or OptionalLong.empty.

showuon · 2022-08-23T06:44:57Z

raft/src/main/java/org/apache/kafka/raft/LeaderState.java

@@ -304,7 +311,7 @@ public long epochStartOffset() {
        return epochStartOffset;
    }

-    private ReplicaState getReplicaState(int remoteNodeId) {
+    private ReplicaState getOrCreateReplicaState(int remoteNodeId) {


nice renaming

showuon · 2022-08-23T06:53:13Z

raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java

+        DescribeQuorumResponseData.ReplicaState leaderState = voterStates.stream()
+            .filter(voterState -> voterState.replicaId() == localId)
+            .findFirst()
+            .orElseThrow(() -> new AssertionError(""));


Forgot to add the error message here and below.

Please update it and below. Thanks.

raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java

showuon · 2022-08-23T07:01:57Z

Also, some MetadataQuorumCommandTest tests failed due to this change. FYI.
cc @dengziming , who just updated the metadata quorum command.

dengziming · 2022-08-23T10:15:16Z

Also, some MetadataQuorumCommandTest tests failed due to this change. FYI.

This is a very simple problem, we changed the output of the tool so the assertion in the test case failed, we only need to adjust the test case.

hachikuji · 2022-08-23T22:29:24Z

Just a general note, I'm repurposing this patch as a general cleanup of the new DescribeQuorum APIs. I changed my mind about HighWatermarkUpdateTimeMs. I think it can be inferred from the existing state, even if imperfectly. I don't feel like knowing the exact value adds that much.

jsancio

Thanks for the changes @hachikuji . I still need to review the tests.

clients/src/main/java/org/apache/kafka/clients/admin/QuorumInfo.java

raft/src/main/java/org/apache/kafka/raft/LeaderState.java

jsancio

LGTM thanks for the improvement @hachikuji

showuon

@hachikuji , thanks for explaining why leaderEpoch change to long. But there are 2 comments you didn't address, yet. Please take a look. Thanks.

showuon · 2022-08-24T02:24:22Z

raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java

+        DescribeQuorumResponseData.ReplicaState leaderState = voterStates.stream()
+            .filter(voterState -> voterState.replicaId() == localId)
+            .findFirst()
+            .orElseThrow(() -> new AssertionError(""));


Please update it and below. Thanks.

raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java

hachikuji · 2022-08-24T17:12:34Z

@showuon Apologies for missing your comments. I pushed an update to address them. Thanks for reviewing!

A few small cleanups in the `DescribeQuorum` API and handling logic: - Change field types in `QuorumInfo`: - `leaderId`: `Integer` -> `int` - `leaderEpoch`: `Integer` -> `long` (to allow for type expansion in the future) - `highWatermark`: `Long` -> `long` - Use field names `lastFetchTimestamp` and `lastCaughtUpTimestamp` consistently - Move construction of `DescribeQuorumResponseData.PartitionData` into `LeaderState` - Consolidate fetch time/offset update logic into `LeaderState.ReplicaState.updateFollowerState` Reviewers: Luke Chen <showuon@gmail.com>, José Armando García Sancio <jsancio@users.noreply.github.com>

showuon · 2022-08-25T02:06:20Z

Thanks @hachikuji , LGTM! Nice refactor!

KAFKA-14142; Expose kraft high watermark update time in quorum command

1a805f9

hachikuji force-pushed the KAFKA-14142 branch from 0fa9955 to 1a805f9 Compare August 23, 2022 00:03

jsancio reviewed Aug 23, 2022

View reviewed changes

clients/src/main/resources/common/message/DescribeQuorumResponse.json Outdated Show resolved Hide resolved

clients/src/main/resources/common/message/DescribeQuorumResponse.json Outdated Show resolved Hide resolved

showuon reviewed Aug 23, 2022

View reviewed changes

hachikuji added 2 commits August 23, 2022 13:50

Walk back addition of high watermark update time

568fa7b

Use timestmap name consistently

bb18033

hachikuji changed the title ~~KAFKA-14142; Expose kraft high watermark update time in quorum command~~ MINOR: A few cleanups for DescribeQuorum APIs Aug 23, 2022

Missed some method renames

5220f88

jsancio reviewed Aug 23, 2022

View reviewed changes

Address review comments

a036a20

jsancio added the 3.3 label Aug 23, 2022

Rename ReplicaState.lastFetchTimeMs to lastFetchTimestamp

baf8869

jsancio approved these changes Aug 23, 2022

View reviewed changes

showuon reviewed Aug 24, 2022

View reviewed changes

Use helper to get replica state in LeaderStateTest

63ee5fd

hachikuji merged commit 5c52c61 into apache:trunk Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: A few cleanups for DescribeQuorum APIs #12548

MINOR: A few cleanups for DescribeQuorum APIs #12548

hachikuji commented Aug 22, 2022 •

edited

jsancio left a comment

showuon left a comment

showuon Aug 23, 2022

showuon Aug 23, 2022

showuon Aug 23, 2022

showuon Aug 23, 2022

showuon Aug 23, 2022

showuon Aug 24, 2022

showuon commented Aug 23, 2022

dengziming commented Aug 23, 2022

hachikuji commented Aug 23, 2022 •

edited

jsancio left a comment

jsancio left a comment

showuon left a comment

showuon Aug 24, 2022

hachikuji commented Aug 24, 2022

showuon commented Aug 25, 2022

	{ "name": "LeaderEpoch", "type": "int32", "versions": "0+",
	"about": "The latest known leader epoch"},

MINOR: A few cleanups for DescribeQuorum APIs #12548

MINOR: A few cleanups for DescribeQuorum APIs #12548

Conversation

hachikuji commented Aug 22, 2022 • edited

Committer Checklist (excluded from commit message)

jsancio left a comment

Choose a reason for hiding this comment

showuon left a comment

Choose a reason for hiding this comment

showuon Aug 23, 2022

Choose a reason for hiding this comment

showuon Aug 23, 2022

Choose a reason for hiding this comment

showuon Aug 23, 2022

Choose a reason for hiding this comment

showuon Aug 23, 2022

Choose a reason for hiding this comment

showuon Aug 23, 2022

Choose a reason for hiding this comment

showuon Aug 24, 2022

Choose a reason for hiding this comment

showuon commented Aug 23, 2022

dengziming commented Aug 23, 2022

hachikuji commented Aug 23, 2022 • edited

jsancio left a comment

Choose a reason for hiding this comment

jsancio left a comment

Choose a reason for hiding this comment

showuon left a comment

Choose a reason for hiding this comment

showuon Aug 24, 2022

Choose a reason for hiding this comment

hachikuji commented Aug 24, 2022

showuon commented Aug 25, 2022

hachikuji commented Aug 22, 2022 •

edited

hachikuji commented Aug 23, 2022 •

edited