Skip to content

KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords#13760

Merged
showuon merged 6 commits intoapache:trunkfrom
tinaselenge:KAFKA-8982
Jul 3, 2023
Merged

KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords#13760
showuon merged 6 commits intoapache:trunkfrom
tinaselenge:KAFKA-8982

Conversation

@tinaselenge
Copy link
Copy Markdown
Contributor

@tinaselenge tinaselenge commented May 24, 2023

Use AdminApiDriver class to refresh the metadata and retry the request that failed with retriable errors.
Modified the unit and integration test cases for LEADER_NOT_AVAILABLE or NOT_LEADER_OR_FOLLOWER as they are now retried using the driver.

Committer Checklist (excluded from commit message)

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

@tinaselenge tinaselenge changed the title KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords Draft: KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords May 25, 2023
@tinaselenge tinaselenge marked this pull request as draft May 25, 2023 12:30
@tinaselenge tinaselenge changed the title Draft: KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords May 25, 2023
@tinaselenge tinaselenge marked this pull request as ready for review May 26, 2023 11:18
Copy link
Copy Markdown
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change! Added some comments on error handling. Otherwise looks good.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tinaselenge , left a meta-comment. Thanks.

Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I made a first quick pass and left a few comments

The test was expecting LeaderNotAvailableException when deleting records on non existant partition.
This test case will no longer fail with this exception because now the metadata request will be retried and the delete records request will timeout instead.
Therefore removed this particular test case so that the test does not wait until timeout.
@tinaselenge
Copy link
Copy Markdown
Contributor Author

@divijvaidya @showuon @mimaison Thank you very much for reviewing the PR!

I believe I have addressed the comments now. Please let me know if I have missed anything. Thanks.

@jlprat jlprat added the admin AdminClient label Jun 12, 2023
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Left some comments.

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated
@showuon
Copy link
Copy Markdown
Member

showuon commented Jun 22, 2023

@tinaselenge , it looks like some tests are failed: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13760/6/
Please also take a look. Thanks.

@tinaselenge
Copy link
Copy Markdown
Contributor Author

Thank you @showuon for the feedback. I think I have addressed the comments. Please let me know there is anything I missed or doesn't look right.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@showuon
Copy link
Copy Markdown
Member

showuon commented Jun 30, 2023

@showuon
Copy link
Copy Markdown
Member

showuon commented Jul 3, 2023

Failed tests are unrelated.

Demogorgon314 pushed a commit to Demogorgon314/kop that referenced this pull request Apr 15, 2026
…mnative#942)

Main changes:
- Adapt to the new `AddPartitionsToTxnRequest` from
apache/kafka#13231 (KIP-890)
- Support the new `DescribeTopicPartitions` request from
apache/kafka#14612 (KIP-966), which is required
by some admin APIs

Other changes:
- apache/kafka#13760 will retry when
`deleteRecords` returns a retriable error, change the error code to
`INVALID_REQUEST`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

admin AdminClient

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants