KAFKA-12541; Extend ListOffset to fetch offset with max timestamp (KIP-734) by thomaskwscott · Pull Request #10760 · apache/kafka

thomaskwscott · 2021-05-25T13:28:49Z

See https://cwiki.apache.org/confluence/display/KAFKA/KIP-734%3A+Improve+AdminClient.listOffsets+to+return+timestamp+and+offset+for+the+record+with+the+largest+timestamp

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Tested with new Integration test

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

… version

dajac · 2021-05-25T13:45:05Z

I will review it in the next few days.

dajac

@thomaskwscott Thanks for the PR. I just made a first read over it and I left few high level questions and few nits.

I feel like we need to add more tests to cover the changes done in the PR. I can think of the following places where we might need to add and/or extend tests:

RequestResponseTest
ListOffsetsRequestTest
KafkaAdminClientTest - Here we need to test all the edge cases.
LogTest - We need to test the changes in Log.
ListOffsetsRequestTest and/or LogOffsetTest

There might other places as well.

dajac · 2021-05-27T09:35:19Z

+        Some(new TimestampAndOffset(latestTimestampSegment.maxTimestampSoFar,
+          latestTimestampSegment.offsetOfMaxTimestampSoFar,
+          epochOptional))


Could we get a maxTimestampSoFar and offsetOfMaxTimestampSoFar which does not correspond to each others? It seems that we have no guarantee here. Is it an issue?

In all cases I can find the 2 are updated together so I think we can assume consistency. For the topic liveness case in the KIP absolute consistency is not required but there will be other cases that will need this (e.g. topic inspection).

I think that we should address this as well. I am fine with doing it in a follow-up PR though so we can keep this focused. Ok for you?

Could we file a JIRA as a subtask in the Jira of the KIP to not forget about it?

sure thing, raised: https://issues.apache.org/jira/browse/KAFKA-12981

dajac · 2021-06-11T06:09:05Z

@thomaskwscott Thanks for the update. I will review it next week.

dajac

@thomaskwscott Thanks for the update. I left few more comments.

dajac

@thomaskwscott Thanks for the update. I left a few more comments/questions.

dajac

@thomaskwscott The PR looks pretty good. I left a few minor comments.

dajac

LGTM, thanks for the patch!

dajac · 2021-06-25T12:25:37Z

Failures are not related:

Build / JDK 16 and Scala 2.13 / kafka.server.ReplicationQuotasTest.shouldThrottleOldSegments()
Build / JDK 16 and Scala 2.13 / org.apache.kafka.streams.integration.KTableKTableForeignKeyJoinDistributedTest.shouldBeInitializedWithDefaultSerde
Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.network.SslTransportLayerTest.[1] tlsProtocol=TLSv1.2, useInlinePem=false
Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.network.SslTransportLayerTest.[2] tlsProtocol=TLSv1.2, useInlinePem=true
Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.network.SslTransportLayerTest.[1] tlsProtocol=TLSv1.2, useInlinePem=false
Build / JDK 8 and Scala 2.12 / org.apache.kafka.common.network.SslTransportLayerTest.[2] tlsProtocol=TLSv1.2, useInlinePem=true
Build / JDK 16 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[1] tlsProtocol=TLSv1.2, useInlinePem=false
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[2] tlsProtocol=TLSv1.2, useInlinePem=true
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[3] tlsProtocol=TLSv1.3, useInlinePem=false
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[1] tlsProtocol=TLSv1.2, useInlinePem=false
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[2] tlsProtocol=TLSv1.2, useInlinePem=true
Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[3] tlsProtocol=TLSv1.3, useInlinePem=false

ijuma · 2021-06-28T14:25:00Z

@vvcephei filed https://issues.apache.org/jira/browse/KAFKA-13002, please take a look and see if it's related to this.

dajac · 2021-06-28T14:49:30Z

Looking at it now.

dajac · 2021-06-28T15:15:53Z

We have found the issue. @thomaskwscott is working on the fix. My bad, I missed one case.

ijuma · 2021-06-28T15:21:51Z

Thanks for the quick investigation!

…TAMP specs (#10936) This patch fixes a regression introduced #10760. The downgrade logic was not downgrading the version when only non MAX_TIMESTAMP specs were used. Reviewers: David Jacot <djacot@confluent.io>

…P-734) (apache#10760) This patch implements KIP-734 as described in https://cwiki.apache.org/confluence/display/KAFKA/KIP-734%3A+Improve+AdminClient.listOffsets+to+return+timestamp+and+offset+for+the+record+with+the+largest+timestamp. Reviewers: David Jacot <djacot@confluent.io>

…TAMP specs (apache#10936) This patch fixes a regression introduced apache#10760. The downgrade logic was not downgrading the version when only non MAX_TIMESTAMP specs were used. Reviewers: David Jacot <djacot@confluent.io>

zhishengzhang · 2022-03-24T08:26:39Z

good job

thomaskwscott added 3 commits May 25, 2021 12:00

KAFKA-12541 add MAX_TIMESTAMP spec to listOffsets api

5a7c00d

Merge remote-tracking branch 'upstream/trunk' into trunk

9cc3c0c

KAFKA-12541 updated replica fetcher to use latest ListOffsets request…

5f5d46b

… version

rajinisivaram requested a review from dajac May 25, 2021 13:31

dajac reviewed May 27, 2021

View reviewed changes

thomaskwscott added 4 commits June 5, 2021 19:13

KAFKA-12541 refactor with retry approach in KafkaAdminClient

4fc4059

Merge remote-tracking branch 'upstream/trunk' into trunk

51aaa70

KAFKA-12541 added LogOffsetTest tests

ae0b6ed

KAFKA-12541 tidy up

24be1c7

thomaskwscott requested a review from dajac June 8, 2021 17:13

dajac reviewed Jun 15, 2021

View reviewed changes

thomaskwscott added 3 commits June 18, 2021 17:36

KAFKA-12541 fixes per pr review

fb85422

KAFKA-12541 adminclient simplification

32e6b2b

KAFKA-12541 refactor of listoffsets retry per pr review comments

a0e9c10

thomaskwscott requested a review from dajac June 21, 2021 15:48

KAFKA-12541 fixes per pr review

32241da

dajac reviewed Jun 21, 2021

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated

dajac reviewed Jun 21, 2021

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated

KAFKA-12541 stopped retries for partitions that cannot use max_timestamp

ea3cd65

dajac reviewed Jun 22, 2021

View reviewed changes

thomaskwscott added 3 commits June 23, 2021 12:36

KAFKA-12541 roll back changes to RequestResponseTest

7355f61

Merge remote-tracking branch 'upstream/trunk' into trunk

b003e84

KAFKA-12541 fixes per pr review

4f4620d

thomaskwscott requested a review from dajac June 23, 2021 12:11

dajac reviewed Jun 24, 2021

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated

KAFKA-12541 fixes per pr review

aada3fc

thomaskwscott requested a review from dajac June 24, 2021 13:50

dajac reviewed Jun 25, 2021

View reviewed changes

Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated

Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated

Comment thread core/src/main/scala/kafka/api/ApiVersion.scala Outdated

KAFKA-12541 fixes per pr review

19f98ab

dajac approved these changes Jun 25, 2021

View reviewed changes

dajac changed the title ~~KAFKA-12541 Extend ListOffset to fetch offset with max timestamp~~ KAFKA-12541; Extend ListOffset to fetch offset with max timestamp (KIP-734) Jun 25, 2021

dajac merged commit bd72ef1 into apache:trunk Jun 25, 2021

dengziming mentioned this pull request Jun 25, 2021

MINOR: Add missing apiversion test for 3.0 #10748

Merged

dajac mentioned this pull request Jun 25, 2021

KAFKA-12609: Rewrite ListOffsets using AdminApiDriver #10467

Closed

3 tasks

Conversation

thomaskwscott commented May 25, 2021

Committer Checklist (excluded from commit message)

Uh oh!

dajac commented May 25, 2021

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dajac May 27, 2021

Choose a reason for hiding this comment

Uh oh!

thomaskwscott May 27, 2021

Choose a reason for hiding this comment

Uh oh!

dajac Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

dajac Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

thomaskwscott Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dajac commented Jun 11, 2021

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Jun 25, 2021

Uh oh!

ijuma commented Jun 28, 2021

Uh oh!

dajac commented Jun 28, 2021

Uh oh!

dajac commented Jun 28, 2021

Uh oh!

ijuma commented Jun 28, 2021

Uh oh!

zhishengzhang commented Mar 24, 2022

Uh oh!

Reviewers