Skip to content

[fix][broker] fix error process in admin.updateTopicPartition #25042

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
TakaHiR07 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[fix][broker] fix error process in admin.updateTopicPartition #25042

TakaHiR07 wants to merge 1 commit into from
+31 −30

Conversation

@TakaHiR07
Copy link
Contributor

@TakaHiR07 TakaHiR07 commented Dec 5, 2025

Main Issue: #25041

Motivation

fix the unreasonable update partition process

Modifications

recover the topic partition process to :

  1. create missing partitions
  2. create subscription for new partition
  3. update topic metadata

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 5, 2025
@BewareMyPower BewareMyPower requested a review from Copilot December 8, 2025 10:58
Copilot AI reviewed Dec 8, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the partition update process in PersistentTopicsBase.internalUpdatePartitionedTopicAsync by reordering operations to prevent inconsistent states. Previously, metadata was updated before creating partitions and subscriptions, which could lead to the metadata indicating more partitions exist than are actually created if subsequent operations failed. The fix ensures metadata is updated only after successfully creating partitions and subscriptions.

Key changes:

  • Reordered the partition update sequence: create partitions → create subscriptions → update metadata (previously: update metadata → create partitions → create subscriptions)
  • Code formatting and indentation improvements for better readability

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mattisonchao
Copy link
Member

mattisonchao commented Dec 11, 2025

Hi @TakaHiR07

Could you help write a test for it?

@TakaHiR07
Copy link
Contributor Author

TakaHiR07 commented Dec 12, 2025 •
edited
Loading

Could you help write a test for it?

@mattisonchao I find that AdminApi2Test#testFailedUpdatePartitionedTopic can not pass. The reason is some other pr such as #24118 on master-branch is implement based on the new process of admin.updateTopicPartition.

It seems that this pr maybe is not a good way for master-branch, but only can use in branch-3.0. Since I don't know how many new pr on master-branch are designed based on new process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

At least 1 approving review is required to merge this pull request.

Assignees

@TakaHiR07 TakaHiR07

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@TakaHiR07 @mattisonchao