KAFKA-15859: Add timeout field to the ListOffsets request by kamalcph · Pull Request #17112 · apache/kafka

kamalcph · 2024-09-06T06:57:37Z

This is the part-3 of the KIP-1075

Added a timeoutMs field to the ListOffsets request. This timeout is applicable only for the topic/partitions that are enabled with remote storage.

When the timeout is defined in the request, then we use it to define the delay timeout for DelayedRemoteListOffsets request. When the timeout is not defined (requests from older client), then we take the dynamic remote.list.offsets.request.timeout.ms server config as the timeout.

Consumer and Admin client behavior are different. Consumer retries the LIST_OFFSETS request in-case of an error but not the AdminClient. And, consumer timeouts the request, if the response exceeds request.timeout.ms, whereas, AdminClient timeouts the request when it exceeds the default.api.timeout.ms.

To retain the same behavior, we are passing the requestTimeoutMs as timeout from the consumer and defaultApiTimeout / overwritten ListOffsetsOption timeout from the admin.

Committer Checklist (excluded from commit message)

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

kamalcph · 2024-09-06T06:58:48Z

This PR is not dependent on #16602 and can be reviewed separately. PTAL.

kamalcph · 2024-09-06T07:03:41Z

-  "validVersions": "0-9",
+  //
+  // Version 10 enables async remote list offsets support (KIP-1075)
+  "validVersions": "0-10",


since we have flexible versions from v6, is it required to bump the version to 10?

Yes, I think this is required.

kamalcph · 2024-09-16T04:48:42Z

This is the final part-3 of KIP-1075. The PR is ready for review. PTAL. Thanks!

satishd

Thanks @kamalcph for the PR, left a minor comment.

showuon

Do you think we should do some guard like this one: KAFKA-17331?

showuon · 2024-09-24T11:53:06Z

-  "validVersions": "0-9",
+  //
+  // Version 10 enables async remote list offsets support (KIP-1075)
+  "validVersions": "0-10",


Yes, I think this is required.

kamalcph · 2024-09-24T13:22:55Z

Do you think we should do some guard like this one: KAFKA-17331?

If the client doesn't provides the timeout, then we will use the remote.list.offsets.request.timeout.ms timeout configured on the server. So, I think it is not required.

kamalcph · 2024-09-25T06:21:40Z

Verified that the timeout propagates as expected by running the server, admin, and consumer locally:

To retain the same behavior, we are passing the requestTimeoutMs as timeout from the consumer and defaultApiTimeout / overwritten ListOffsetsOption timeout from the admin.

showuon

LGTM!

satishd

Thanks @kamalcph for addressing the review comments. LGTM.

mumrah · 2024-09-26T15:40:47Z

@satishd @showuon this PR broke some tests and has caused trunk builds to fail.

The latest CI run for this PR was https://github.com/apache/kafka/actions/runs/11027485627 which shows both of the JUnit steps failing.

Unlike with Jenkins, we need to look at every failed test build -- especially before merging. Looking at these two failed jobs, we can see we have actual failed tests and not just flaky ones

https://github.com/apache/kafka/actions/runs/11027485627/job/30626323008

We want to see all the status checks in the PR to be green before merging. I would like to have branch protections in place to prevent this sort of regression, but until we sort out the flaky tests we can't really.

Anyways, I'm not meaning to pick on this PR -- just trying to raise awareness of our "new normal" for build expectations :)

kamalcph · 2024-09-26T15:50:51Z

The failed test are fixed in #17287

satishd · 2024-09-27T03:59:32Z

Sorry for missing the tests(thought those were the flaky ones). Thanks @mumrah for following up and getting the fix merged.

lucasbru · 2024-10-02T17:34:44Z

@kamalcph We were using current trunk with brokers that do not have your change yet, and AdminClient's ListOffset call is running into

org.apache.kafka.common.errors.UnsupportedVersionException: Attempted to write a non-default timeoutMs at version 8

It seems this could be related to this PR. Could you please take a look?

kamalcph · 2024-10-03T10:02:10Z

org.apache.kafka.common.errors.UnsupportedVersionException: Attempted to write a non-default timeoutMs at version 8

Opened #17358 to address this issue. PTAL.

This is part-3 of the KIP-1075. Added a `timeoutMs` field to the ListOffsets request. This timeout is applicable only for the topic/partitions that are enabled with remote storage. When the timeout is defined in the request, then we use it to define the delay timeout for `DelayedRemoteListOffsets` request. When the timeout is not defined (requests from older client), then we take the dynamic `remote.list.offsets.request.timeout.ms` server config as the timeout. Consumer and Admin client behavior are different. Consumer retries the LIST_OFFSETS request in-case of an error but not the AdminClient. And, consumer timeouts the request, if the response exceeds `request.timeout.ms`, whereas, AdminClient timeouts the request when it exceeds the `default.api.timeout.ms`. To retain the same behavior, we are passing the `requestTimeoutMs` as timeout from the consumer and defaultApiTimeout / overwritten ListOffsetsOption timeout from the admin. Reviewers: Satish Duggana <satishd@apache.org>, Luke Chen <showuon@gmail.com>

kamalcph requested review from chia7712, clolov, satishd and showuon September 6, 2024 06:57

kamalcph added the tiered-storage Related to the Tiered Storage feature label Sep 6, 2024

kamalcph commented Sep 6, 2024

View reviewed changes

kamalcph force-pushed the KAFKA-15859b branch from fbede17 to 77671e3 Compare September 6, 2024 09:20

kamalcph mentioned this pull request Sep 9, 2024

KAFKA-15859: Make RemoteListOffsets call an async operation #16602

Merged

3 tasks

kamalcph force-pushed the KAFKA-15859b branch 3 times, most recently from 1a91702 to c0f94be Compare September 16, 2024 04:46

satishd reviewed Sep 16, 2024

View reviewed changes

Comment thread clients/src/main/resources/common/message/ListOffsetsRequest.json Outdated

kamalcph force-pushed the KAFKA-15859b branch 2 times, most recently from 61ded02 to 4c274e8 Compare September 19, 2024 13:56

showuon reviewed Sep 24, 2024

View reviewed changes

kamalcph force-pushed the KAFKA-15859b branch from 4c274e8 to 4a160ec Compare September 24, 2024 13:16

github-actions Bot added core Kafka Broker consumer clients labels Sep 24, 2024

kamalcph added 3 commits September 25, 2024 11:53

KAFKA-15859: Add timeout field to the ListOffsets request

3134ef1

update field description

abf2254

Bump default IBP from 4_0_IV2 to 4_0_IV3

1cff7ec

kamalcph force-pushed the KAFKA-15859b branch from 4a160ec to 1cff7ec Compare September 25, 2024 06:23

showuon approved these changes Sep 25, 2024

View reviewed changes

satishd approved these changes Sep 26, 2024

View reviewed changes

satishd merged commit 560076b into apache:trunk Sep 26, 2024

kamalcph deleted the KAFKA-15859b branch September 26, 2024 10:42

chia7712 mentioned this pull request Sep 26, 2024

HOTFIX: fix failed cases in FeatureCommandTest #17287

Merged

3 tasks

Conversation

kamalcph commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

kamalcph commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamalcph Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

showuon Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

kamalcph commented Sep 16, 2024

Uh oh!

satishd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

showuon Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamalcph commented Sep 24, 2024

Uh oh!

kamalcph commented Sep 25, 2024

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

satishd left a comment

Choose a reason for hiding this comment

Uh oh!

mumrah commented Sep 26, 2024

Uh oh!

kamalcph commented Sep 26, 2024

Uh oh!

satishd commented Sep 27, 2024

Uh oh!

lucasbru commented Oct 2, 2024

Uh oh!

kamalcph commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kamalcph commented Sep 6, 2024 •

edited

Loading

kamalcph commented Sep 6, 2024 •

edited

Loading

kamalcph commented Oct 3, 2024 •

edited

Loading