You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/08/05 03:59:49 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
AnonHxy opened a new pull request, #16953:
URL: https://github.com/apache/pulsar/pull/16953
Fixes https://github.com/apache/pulsar/issues/16952
### Motivation
* Fix ConcurrentModificationException when ModularLoadManagerImpl start. The root cause is that `ModularLoadManagerImpl#knownBrokers` can be modified in multi-thread through `ModularLoadManagerImpl#updateAll`
* Thread-1 is line290:
https://github.com/apache/pulsar/blob/5c09d536304e5873c4e1ceeb8353a1150b5d61f4/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java#L282-L292
* Thread-2 is line969
https://github.com/apache/pulsar/blob/5c09d536304e5873c4e1ceeb8353a1150b5d61f4/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java#L938-L970
### Modifications
* Changing `ModularLoadManagerImpl#knownBrokers` to `ConcurrentHashMap.newKeySet()`
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16953:
URL: https://github.com/apache/pulsar/pull/16953#discussion_r939605574
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java:
##########
@@ -197,7 +197,7 @@ public class ModularLoadManagerImpl implements ModularLoadManager {
private long unloadBundleCount = 0;
private final Lock lock = new ReentrantLock();
- private Set<String> knownBrokers = new HashSet<>();
+ private Set<String> knownBrokers = ConcurrentHashMap.newKeySet();
Review Comment:
`handleDataNotification ` also has other actions, so we'd better keep it in `init` method I think @poorbarcode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16953:
URL: https://github.com/apache/pulsar/pull/16953#issuecomment-1207341632
@poorbarcode Could you do the final review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on pull request #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16953:
URL: https://github.com/apache/pulsar/pull/16953#issuecomment-1206107772
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16953:
URL: https://github.com/apache/pulsar/pull/16953#discussion_r938617143
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java:
##########
@@ -197,7 +197,7 @@ public class ModularLoadManagerImpl implements ModularLoadManager {
private long unloadBundleCount = 0;
private final Lock lock = new ReentrantLock();
- private Set<String> knownBrokers = new HashSet<>();
+ private Set<String> knownBrokers = ConcurrentHashMap.newKeySet();
Review Comment:
Maybe we can move this line below to method `start` and add some code comments, this could solves the concurrency problem.
https://github.com/apache/pulsar/blob/7650612cd86ead01886300f9b31aba6946831736/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java#L249
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on pull request #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #16953:
URL: https://github.com/apache/pulsar/pull/16953#issuecomment-1207341913
> @poorbarcode Could you do the final review?
OK. Approved
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #16953: [fix][broker]Fix ConcurrentModificationException when ModularLoadManagerImpl start
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #16953:
URL: https://github.com/apache/pulsar/pull/16953
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org