You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/04/20 23:42:59 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #8572: Fix the deadlock in ClusterChangeMediator
Jackie-Jiang opened a new pull request, #8572:
URL: https://github.com/apache/pinot/pull/8572
Fix the potential deadlock in `ClusterChangeMediator` between Helix callback handling thread and ClusterChangeMediator change handling thread when the ClusterChangeMediator is stopped.
The deadlock chain is as following:
- ClusterChangeMediator.stop() is called and waiting for the change handling thread to stop
- Helix callback handling thread acquires a lock, then send a cluster change to the mediator which is blocked ClusterChangeMediator.stop() (both stop() and enqueueChange() are synchronized)
- The change handling thread waits on the lock held by the Helix callback handling thread
Changes:
- Reduce the span of the synchronized block
- Change the stop to use interrupt instead of notify
- Add a test to guard the deadlock
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] PrachiKhobragade commented on a diff in pull request #8572: Fix the deadlock in ClusterChangeMediator
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on code in PR #8572:
URL: https://github.com/apache/pinot/pull/8572#discussion_r854675385
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/ClusterChangeMediator.java:
##########
@@ -210,21 +206,21 @@ public void onLiveInstanceChange(List<LiveInstance> liveInstances, NotificationC
*
* @param changeType Type of the change
*/
- private synchronized void enqueueChange(ChangeType changeType) {
+ private void enqueueChange(ChangeType changeType) {
// Do not enqueue or process changes if already stopped
- if (_stopped) {
+ if (!_running) {
+ LOGGER.warn("ClusterChangeMediator already stopped, skipping enqueuing the {} change", changeType);
return;
}
if (_clusterChangeHandlingThread.isAlive()) {
- LOGGER.info("Enqueue {} change", changeType);
- synchronized (_lastChangeTimeMap) {
- if (!_lastChangeTimeMap.containsKey(changeType)) {
- _lastChangeTimeMap.put(changeType, System.currentTimeMillis());
+ LOGGER.info("Enqueueing {} change", changeType);
+ if (_lastChangeTimeMap.putIfAbsent(changeType, System.currentTimeMillis()) == null) {
+ synchronized (_lastChangeTimeMap) {
_lastChangeTimeMap.notify();
Review Comment:
Always use notifyAll()
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] PrachiKhobragade commented on a diff in pull request #8572: Fix the deadlock in ClusterChangeMediator
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on code in PR #8572:
URL: https://github.com/apache/pinot/pull/8572#discussion_r855616042
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/ClusterChangeMediator.java:
##########
@@ -210,21 +206,21 @@ public void onLiveInstanceChange(List<LiveInstance> liveInstances, NotificationC
*
* @param changeType Type of the change
*/
- private synchronized void enqueueChange(ChangeType changeType) {
+ private void enqueueChange(ChangeType changeType) {
// Do not enqueue or process changes if already stopped
- if (_stopped) {
+ if (!_running) {
+ LOGGER.warn("ClusterChangeMediator already stopped, skipping enqueuing the {} change", changeType);
return;
}
if (_clusterChangeHandlingThread.isAlive()) {
- LOGGER.info("Enqueue {} change", changeType);
- synchronized (_lastChangeTimeMap) {
- if (!_lastChangeTimeMap.containsKey(changeType)) {
- _lastChangeTimeMap.put(changeType, System.currentTimeMillis());
+ LOGGER.info("Enqueueing {} change", changeType);
+ if (_lastChangeTimeMap.putIfAbsent(changeType, System.currentTimeMillis()) == null) {
+ synchronized (_lastChangeTimeMap) {
_lastChangeTimeMap.notify();
Review Comment:
I don't think notifyAll will be required in single threaded case. Thanks for clarifying.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8572: Fix the deadlock in ClusterChangeMediator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8572:
URL: https://github.com/apache/pinot/pull/8572#discussion_r854759946
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/ClusterChangeMediator.java:
##########
@@ -210,21 +206,21 @@ public void onLiveInstanceChange(List<LiveInstance> liveInstances, NotificationC
*
* @param changeType Type of the change
*/
- private synchronized void enqueueChange(ChangeType changeType) {
+ private void enqueueChange(ChangeType changeType) {
// Do not enqueue or process changes if already stopped
- if (_stopped) {
+ if (!_running) {
+ LOGGER.warn("ClusterChangeMediator already stopped, skipping enqueuing the {} change", changeType);
return;
}
if (_clusterChangeHandlingThread.isAlive()) {
- LOGGER.info("Enqueue {} change", changeType);
- synchronized (_lastChangeTimeMap) {
- if (!_lastChangeTimeMap.containsKey(changeType)) {
- _lastChangeTimeMap.put(changeType, System.currentTimeMillis());
+ LOGGER.info("Enqueueing {} change", changeType);
+ if (_lastChangeTimeMap.putIfAbsent(changeType, System.currentTimeMillis()) == null) {
+ synchronized (_lastChangeTimeMap) {
_lastChangeTimeMap.notify();
Review Comment:
I understand that using `notifyAll()` can prevent certain deadlock situations, but in the current code there is only one single thread that can wait on this lock. Do you see a problem using `notify()` in this case?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8572: Fix the deadlock in ClusterChangeMediator
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8572:
URL: https://github.com/apache/pinot/pull/8572#issuecomment-1104569756
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8572?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8572](https://codecov.io/gh/apache/pinot/pull/8572?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c135397) into [master](https://codecov.io/gh/apache/pinot/commit/6eff17754a0b0ffdfb8670b54b5ada38961b7917?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6eff177) will **decrease** coverage by `56.72%`.
> The diff coverage is `77.50%`.
```diff
@@ Coverage Diff @@
## master #8572 +/- ##
=============================================
- Coverage 70.83% 14.11% -56.73%
+ Complexity 4307 84 -4223
=============================================
Files 1688 1644 -44
Lines 88295 86519 -1776
Branches 13358 13184 -174
=============================================
- Hits 62542 12208 -50334
- Misses 21388 73396 +52008
+ Partials 4365 915 -3450
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.11% <77.50%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...not/broker/broker/helix/ClusterChangeMediator.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0NsdXN0ZXJDaGFuZ2VNZWRpYXRvci5qYXZh) | `74.19% <77.50%> (-0.28%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1348 more](https://codecov.io/gh/apache/pinot/pull/8572/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8572?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8572?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6eff177...c135397](https://codecov.io/gh/apache/pinot/pull/8572?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8572: Fix the deadlock in ClusterChangeMediator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8572:
URL: https://github.com/apache/pinot/pull/8572#discussion_r855416302
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/ClusterChangeMediator.java:
##########
@@ -210,21 +206,21 @@ public void onLiveInstanceChange(List<LiveInstance> liveInstances, NotificationC
*
* @param changeType Type of the change
*/
- private synchronized void enqueueChange(ChangeType changeType) {
+ private void enqueueChange(ChangeType changeType) {
// Do not enqueue or process changes if already stopped
- if (_stopped) {
+ if (!_running) {
+ LOGGER.warn("ClusterChangeMediator already stopped, skipping enqueuing the {} change", changeType);
return;
}
if (_clusterChangeHandlingThread.isAlive()) {
- LOGGER.info("Enqueue {} change", changeType);
- synchronized (_lastChangeTimeMap) {
- if (!_lastChangeTimeMap.containsKey(changeType)) {
- _lastChangeTimeMap.put(changeType, System.currentTimeMillis());
+ LOGGER.info("Enqueueing {} change", changeType);
+ if (_lastChangeTimeMap.putIfAbsent(changeType, System.currentTimeMillis()) == null) {
+ synchronized (_lastChangeTimeMap) {
_lastChangeTimeMap.notify();
Review Comment:
Merging as is for now. We may open a separate PR if `notifyAll()` should be used instead of `notify()`
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #8572: Fix the deadlock in ClusterChangeMediator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #8572:
URL: https://github.com/apache/pinot/pull/8572
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org