Skip to content
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.

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

[fix][ml] Fix wrong configuration passed to managed ledger when deleting #21529

Closed
wants to merge 1 commit into from
Closed

[fix][ml] Fix wrong configuration passed to managed ledger when deleting #21529

wants to merge 1 commit into from

Conversation

mattisonchao
Copy link
Member

Motivation

#21093 Delete the topic policy before the managed ledger configuration building. It will pass the wrong managed ledger configuration to the backend and affect offloading etc.

Modifications

  • Get managed ledger configuration before deleting topic policies.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

@mattisonchao mattisonchao self-assigned this Nov 7, 2023
@mattisonchao mattisonchao added the category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost label Nov 7, 2023
@mattisonchao mattisonchao added this to the 3.2.0 milestone Nov 7, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 7, 2023
@mattisonchao mattisonchao reopened this Nov 7, 2023
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test to avoid regressions?

@mattisonchao
Copy link
Member Author

I am unsure at first glance. I think the test is challenging to add. I am considering whether it is worth to spend that time. :/

@codelipenghui, do you think this test is necessary? I mean the problem looks pretty straightforward.

@mattisonchao
Copy link
Member Author

I will add the test to avoid regression. :)

@Technoboy-
Copy link
Contributor

topicPoliciesFuture throw exception because race condition occurs in getManagedLedgerConfig.
Need to handle exception for topicPoliciesFuture

@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@mattisonchao mattisonchao closed this by deleting the head repository Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.3 release/3.1.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants