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.

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

[Issue 12726][broker] Fix deadlock in metadata-store callback thread #12753

Merged
merged 7 commits into from
Nov 22, 2021
Merged

[Issue 12726][broker] Fix deadlock in metadata-store callback thread #12753

merged 7 commits into from
Nov 22, 2021

Conversation

Jason918
Copy link
Contributor

@Jason918 Jason918 commented Nov 11, 2021 •
edited
Loading

Fixes #12726

Motivation

See #12726

Modifications

Use PolicyHierarchyValue to cache backlogQuota policies to avoid calling blocking metadata query in metadata-callback thread.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc
    Bug fix.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2021
@codelipenghui codelipenghui added this to the 2.10.0 milestone Nov 11, 2021
codelipenghui
codelipenghui reviewed Nov 11, 2021
View reviewed changes
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/HierarchyTopicPolicies.java Outdated

public HierarchyTopicPolicies() {
inactiveTopicPolicies = new PolicyHierarchyValue<>();
maxSubscriptionsPerTopic = new PolicyHierarchyValue<>();
backLogQuotaMap = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the map be accessed by multiple threads?

And I think it should be PolicyHierarchyValue<Map<BacklogQuota.BacklogQuotaType, BacklogQuota>>?

Copy link
Contributor Author

@Jason918 Jason918 Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can the map be accessed by multiple threads?

Yes, thanks for the reminding. ImmutableMap is a better choice here.

And I think it should be PolicyHierarchyValue<Map<BacklogQuota.BacklogQuotaType, BacklogQuota>>?

With this, there will be a problem under this case:

TopicLevelSettings : destination_storage --> BacklogQuota1, message_age --> null
BrokerSettings : destination_storage --> null, message_age --> BacklogQuota2

With PolicyHierarchyValue<Map<...>>, the value of type message_age would be null.
With Map, the value will be "BacklogQuota2".
In previous logic, the result seems to be "BacklogQuota2".
I will add a test case to verify this.

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

4 similar comments
@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

Jason918 commented Nov 14, 2021 •
edited
Loading

@codelipenghui Updated, please help review again.

  1. Update backLogQuotaMap to ImuutableMap, should be thread safe.
  2. Update a test that show PolicyHierarchyValue<Map<BacklogQuota.BacklogQuotaType, BacklogQuota>> will not work in pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java, line 250
  3. Fix some unit test failure

@Jason918
Copy link
Contributor Author

Jason918 commented Nov 15, 2021 •
edited
Loading

@315157973 Would you please help review this?

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

@315157973 Fixed the previous bug. PTAL.

@Jason918
Copy link
Contributor Author

@codelipenghui Can you help merge this please?

@codelipenghui codelipenghui merged commit ebfcd71 into apache:master Nov 22, 2021
dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Nov 23, 2021
@Jason918 @dlg99
[Issue 12726][broker] Fix deadlock in metadata-store callback thread (a…
2a56bce 
…pache#12753)

Fixes apache#12726

### Motivation

See apache#12726 


### Modifications

Use PolicyHierarchyValue to cache backlogQuota policies to avoid calling blocking metadata query in metadata-callback thread.
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
@Jason918 @eolivelli
[Issue 12726][broker] Fix deadlock in metadata-store callback thread (a…
f5d4b00 
…pache#12753)

Fixes apache#12726

### Motivation

See apache#12726 


### Modifications

Use PolicyHierarchyValue to cache backlogQuota policies to avoid calling blocking metadata query in metadata-callback thread.
@Jason918 Jason918 mentioned this pull request Dec 4, 2021
Merged
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@Jason918
[Issue 12726][broker] Fix deadlock in metadata-store callback thread (a…
3607093 
…pache#12753)

Fixes apache#12726

### Motivation

See apache#12726 


### Modifications

Use PolicyHierarchyValue to cache backlogQuota policies to avoid calling blocking metadata query in metadata-callback thread.
@Jason918 Jason918 mentioned this pull request Dec 21, 2021
Merged
2 tasks
codelipenghui pushed a commit that referenced this pull request Dec 21, 2021
@Jason918
[broker] Fix deadlock in metadata-store callback thread for branch 2.9 (
c5a4639 
#13426)

Fixes #12726 for branch 2.9

### Motivation

See #12726

#12753 fixed this in master, but can not merged into branch 2.9.
@Jason918 Jason918 mentioned this pull request Jan 14, 2022
Closed
23 tasks
@sijie sijie mentioned this pull request Jan 14, 2022
Closed
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@315157973 315157973 315157973 approved these changes

Assignees

@Jason918 Jason918

Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

Deadlock in metadata-store callback thread
4 participants
@Jason918 @315157973 @codelipenghui @eolivelli