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][broker] Revert the async modification cause broker lost bookie's rack information in the PR#22846 #23283

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

Conversation

TakaHiR07
Copy link
Contributor

Main Issue: #23282

Motivation

Related to #22846 and #22853.

I guess there is a modification in previous pr cause broker lost bookie rack information permanently. The analysis process can see the issue.

Modifications

revert the change.

Verifying this change

  • Make sure that the change passes the CI checks.

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 11, 2024
@lhotari
Copy link
Member

lhotari commented Sep 23, 2024

Closing this PR since #23331 replaces this.

@lhotari lhotari closed this Sep 23, 2024
@TakaHiR07
Copy link
Contributor Author

TakaHiR07 commented Sep 23, 2024

Closing this PR since #23331 replaces this.

@lhotari This pr can not be closed. Because #23331 is not a replaced one, that pr can not fix this issue's error.

@lhotari
Copy link
Member

lhotari commented Sep 23, 2024

Closing this PR since #23331 replaces this.

@lhotari This pr can not be closed. Because #23331 is not a replaced one, that pr can not fix this issue's error.

@TakaHiR07 ok, I'll reopen this. However we cannot simply revert since there's another issue of deadlocks that appears if that is done. Please start the dev mailing list discussion (as discussed in #23330 (comment)) since this problem is a release blocker.

@lhotari lhotari reopened this Sep 23, 2024
@TakaHiR07
Copy link
Contributor Author

@TakaHiR07 ok, I'll reopen this. However we cannot simply revert since there's another issue of deadlocks that appears if that is done. Please start the dev mailing list discussion (as discussed in #23330 (comment)) since this problem is a release blocker.

I have noticed the deadlock issue before and I am not sure why fix deadlock should make BookieRackAffinityMapping#watchAvailableBookies become async. I guess we just need this pr, then can fix the deadlock, #22853

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants