KAFKA-13888: Addition of Information in DescribeQuorumResponse by niket-goel · Pull Request #12508 · apache/kafka

niket-goel · 2022-08-12T00:51:01Z

This commit adds in the implementation for two new fields:

LastFetchTimestamp
LastCaughtUpTimestamp

as they are described in KIP-836

This commit adds in the implementation for two new fields: * LastFetchTimestamp * LastCaughtUpTimestamp as they are described in KIP-836

hachikuji · 2022-08-15T19:59:21Z

-            convertToReplicaStates(leaderState.getVoterEndOffsets()),
-            convertToReplicaStates(leaderState.getObserverStates(currentTimeMs))
+            convertToReplicaStates(leaderState.getVoterEndOffsets(),
+                                    leaderState.getVoterLastFetchTimes(),


I think it would be a bit simpler to push creation of the ReplicaState object into LeaderState:

class LeaderState { ... List<ReplicaState> voterStates(long currentTimeMs); List<ReplicaState> observerStates(long currentTimeMs); }

Then we wouldn't need all the boilerplate methods to create all the different Map variations. Also, then the sanity checks in convertToReplicaStates become unnecessary.

hachikuji · 2022-08-15T20:11:02Z

-        state.updateFetchTimestamp(fetchTimestamp);
+        // Update the Last CaughtUp Time
+        if (logOffsetMetadata.offset >= leaderLogEndOffset) {
+            state.updateLastCaughtUpTimestamp(fetchTimestamp);


We do these updates before the call to updateEndOffset, which may raise an exception (same thing for updateFetchTimestamp which is an existing issue). I think it would be better to update state only after we have confirmed the update is valid.

hachikuji · 2022-08-15T20:15:18Z

+        val quorumInfo = quorumState.quorumInfo.get()
+
+        assertEquals(0, quorumInfo.observers.size)
+        assertEquals(3, quorumInfo.voters.size)


A more straightforward way to assert the voter/observer sets is in DescribeQuorumRequestTest:

val voterData = partitionData.currentVoters.asScala assertEquals(cluster.controllerIds().asScala, voterData.map(_.replicaId).toSet); val observerData = partitionData.observers.asScala assertEquals(cluster.brokerIds().asScala, observerData.map(_.replicaId).toSet);

That check already exists in the DescribeQuorumRequestTest. The count validation here is just a sanity check.

We are checking the size here and all of the individual replicaIds below. For example:

assertTrue(-1 < observer.replicaId && 4 > observer.replicaId, s"Observer ID ${observer.replicaId} was not within expected range.")

I am just offering a more concise and complete way to do that.

You may have missed my comment above. The nice thing about asserting the set directly is that it ensures that all expected voters/observers are present in the result.

* Some code refactoring * Fixing error handling when updating LeaderState::ReplicaState

hachikuji · 2022-08-16T21:05:26Z

+        val quorumInfo = quorumState.quorumInfo.get()
+
+        assertEquals(0, quorumInfo.observers.size)
+        assertEquals(3, quorumInfo.voters.size)


You may have missed my comment above. The nice thing about asserting the set directly is that it ensures that all expected voters/observers are present in the result.

hachikuji · 2022-08-16T22:36:40Z

-        assertEquals(4, quorumInfo.observers.size)
-        assertEquals(3, quorumInfo.voters.size)
+        assertEquals(cluster.controllers.asScala.keySet, quorumInfo.voters.asScala.map(_.replicaId).toSet)
        assertTrue(2999 < quorumInfo.leaderId && 3003 > quorumInfo.leaderId,


We could make this assertion a little more generic by asserting that quorumInfo.leaderId is contained in cluster.controllers.asScala.keySet.

hachikuji · 2022-08-16T22:46:40Z

+    private boolean updateEndOffset(
+        ReplicaState state,
+        LogOffsetMetadata endOffsetMetadata,
+        boolean verifyUpdate


Why do we need this? Seems like false is the only value we ever pass.

Wanted to retain the verification step in the method in case we want to use it elsewhere in the future.

hachikuji

Thanks, just a few more small comments.

niket-goel · 2022-08-17T18:54:12Z

Thanks for the comments @hachikuji . Have addressed them and pushed another version. :)

hachikuji

LGTM. Just one more nit to fix.

hachikuji · 2022-08-17T23:52:33Z

+          assertNotEquals(OptionalLong.empty(), observer.lastCaughtUpTimeMs)
+        }
+      } catch {
+        case t: Throwable => throw t


nit: if we're just re-throwing, we don't need to catch

hachikuji

Thanks, LGTM. We'll have to be a bit careful since #12469 is also ready to merge. We'll see whoever gets the clean build first.

This reverts commit a12013c.

hachikuji

Thanks, the updates LGTM as well. Hopefully we've nailed the cause of the test flakiness.

…tamp` for DescribeQuorumResponse [KIP-836] (#12508) This commit implements the newly added fields `LastFetchTimestamp` and `LastCaughtUpTimestamp` for KIP-836: https://cwiki.apache.org/confluence/display/KAFKA/KIP-836:+Addition+of+Information+in+DescribeQuorumResponse+about+Voter+Lag. Reviewers: Jason Gustafson <jason@confluent.io>

KAFKA-13888: Addition of Information in DescribeQuorumResponse

c5b4323

This commit adds in the implementation for two new fields: * LastFetchTimestamp * LastCaughtUpTimestamp as they are described in KIP-836

hachikuji reviewed Aug 15, 2022

View reviewed changes

niket-goel added 2 commits August 16, 2022 09:02

Merge branch 'trunk' into kafka-13888-describe-quorum-fields

19af2c2

Addressing Review Comments

ddbfbea

* Some code refactoring * Fixing error handling when updating LeaderState::ReplicaState

niket-goel marked this pull request as ready for review August 16, 2022 18:12

hachikuji reviewed Aug 16, 2022

View reviewed changes

Comment thread raft/src/main/java/org/apache/kafka/raft/LeaderState.java Outdated

More Refactoring based on review comments

0d70f91

hachikuji reviewed Aug 16, 2022

View reviewed changes

Styling Changes and some test improvement based on review

73e034e

hachikuji reviewed Aug 16, 2022

View reviewed changes

niket-goel added 2 commits August 16, 2022 16:30

Removing unused variable from test method

5de54cd

Removing an unused argument from updateEndOffset

9322538

hachikuji reviewed Aug 17, 2022

View reviewed changes

more refactoring

36f4f75

niket-goel added 2 commits August 17, 2022 16:02

Merge branch 'trunk' into kafka-13888-describe-quorum-fields

8630b3f

Removing check for Kafka-13940 in test as it has been fixed

b1c2b1e

hachikuji reviewed Aug 17, 2022

View reviewed changes

Fixing build failure and removing unnecessary catch

9a55624

hachikuji approved these changes Aug 18, 2022

View reviewed changes

niket-goel added 6 commits August 18, 2022 10:22

Fixing failure in DescribeQuorumRequestTest

8a105d2

Merge branch 'trunk' into kafka-13888-describe-quorum-fields

0d79978

Fixing breaking test in KafkaRaftClientTest

b00e66c

Removing falky integration test

a12013c

Revert "Removing falky integration test"

02dc7ad

This reverts commit a12013c.

Return NOT_CONTROLLER if forwarded DescribeQuorum has NOT_LEADER error

d1936ab

niket-goel force-pushed the kafka-13888-describe-quorum-fields branch from 38a0af7 to d1936ab Compare August 19, 2022 18:48

hachikuji approved these changes Aug 19, 2022

View reviewed changes

hachikuji merged commit c7f0519 into apache:trunk Aug 19, 2022

niket-goel mentioned this pull request Aug 31, 2022

Kafka 13888 new fields #12212

Closed

3 tasks

Conversation

niket-goel commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niket-goel commented Aug 17, 2022

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

niket-goel commented Aug 12, 2022 •

edited

Loading

hachikuji Aug 16, 2022 •

edited

Loading