You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/06/26 02:02:13 UTC
[GitHub] [bookkeeper] horizonzy opened a new pull request, #3365: Fix race condition.
horizonzy opened a new pull request, #3365:
URL: https://github.com/apache/bookkeeper/pull/3365
### Motivation
fixes-#3362
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#issuecomment-1166946619
> Overall look good to me. Could you please add a unit test to cover this?
fine.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #3365: Fix race condition.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#issuecomment-1434026289
rerun 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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r911609146
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
with this
```
Set<LedgerMetadataListener> listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
synchronized (listenerSet) {
listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
listenerSet.add(listener);
}
```
it looks like the codes assume that between two calls of `listeners.computeIfAbsent` it may need to create a new instance (listeners can be modified/removed concurrently.)
so you are getting (or creating) one instance to synchronize on, meanwhile, listeners change, now synchronize locks on one instance of listenerSet while modifying another.
so either the second call to computeIfAbsent is not needed or the locking needs to be done differently
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r929552306
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
It will reduce too many concurrency. If we listen too many zk nodes, when received event, also synchronized, it maybe make program slow.
see https://github.com/apache/bookkeeper/blob/ded05aa2218f1e98c650070e9d0152213c4d09fe/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java#L125.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix race condition. [bookkeeper]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap closed pull request #3365: Fix race condition.
URL: https://github.com/apache/bookkeeper/pull/3365
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#issuecomment-1169476939
> I left couple of comments to address.
>
> As for the original issue, is this a theoretical find or from a production case? Most of the ledger-specific mutations in BK happen in the ordered executor where the ledgerid is used as a key, so the mutations should happen on the same thread sequentially.
> I left couple of comments to address.
>
> As for the original issue, is this a theoretical find or from a production case? Most of the ledger-specific mutations in BK happen in the ordered executor where the ledgerid is used as a key, so the mutations should happen on the same thread sequentially.
This is not real case, just for analyze. We can't ensure the user open ledger and close ledger in same thread, so if you want to process register and unregister serially, should handle it in AbstractZkLedgerManageer, not outside by invoke user.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#issuecomment-1166468725
> Nice catch!
>
> Please add more details about how the race condition happened.
see #3362, update the detail info
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r909157231
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
> I think this should be before the synchronized block, not inside of it
Maybe not, if outside, the race condition problem still exist.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix race condition. [bookkeeper]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#issuecomment-1837768043
Close this since it is open for a long time without any updates. Feel free to reopen it if you want to continue
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r908805581
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
with this, do we still need the lines above:
```
Set<LedgerMetadataListener> listenerSet = listeners.get(ledgerId);
if (listenerSet == null) {
Set<LedgerMetadataListener> newListenerSet = new HashSet<LedgerMetadataListener>();
Set<LedgerMetadataListener> oldListenerSet = listeners.putIfAbsent(ledgerId, newListenerSet);
if (null != oldListenerSet) {
listenerSet = oldListenerSet;
} else {
listenerSet = newListenerSet;
}
}
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
I think this should be before the synchronized block, not inside of it
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r929509298
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
Let's use lock object (`private final Object lock = new Object();` is good enough); and replace other instance of `synchronized (listenerSet)` with `synchronized (lock) ...`
Otherwise I think it will just make the problem harder to reproduce
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#issuecomment-1225340346
fix old workflow,please see #3455 for detail
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r909157330
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
This logic can be simply.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3365: Fix race condition.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r911624054
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
}
}
synchronized (listenerSet) {
+ listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
Review Comment:
If we don't want second `computeIfAbsent` and still lock by ledgerId, we should define other object which is immutable. likes Map<String, Object> key: ledgerId, value: empty object. But it will introduce more object.
The simplest way is make register and unregister synchronize, but it will reduce concurrency.
In summary, I think second `computeIfAbsent` is the relatively good way.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org