lhotari commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1888561697
> 1. So we not use the OrderedExecutor, and should synchronized
(pendingAddOps) for serializing drainPendingAddsAndAdjustLength &
sendAddSuccessCallbacks. Is it right? @lhotari
graysonzeng commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1888508379
> I created an alternative fix #4175 to bring clarity of what I'd be
proposing. Once we have a reproducer for the issue in BK as a unit test, it
will be easier to validate.
graysonzeng commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1888338104
@lhotari @hangc0276 @eolivelli Thank you very much for your suggestions,
I'll be happy to continue improving it.
> True, but that would be a significant change that could at
graysonzeng commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1888308176
@lhotari @hangc0276 @eolivelli Thank you very much for your suggestions,
I'll be happy to continue improving it.
> True, but that would be a significant change that could at
lhotari commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1888025418
I created an alternative fix #4175 to bring clarity of what I'd be
proposing. Once we have a reproducer for the issue in BK as a unit test, it
will be easier to validate.
--
This
lhotari opened a new pull request, #4175:
URL: https://github.com/apache/bookkeeper/pull/4175
### Motivation
There's a race condition in LedgerHandle between
drainPendingAddsAndAdjustLength & sendAddSuccessCallbacks method calls. The
methods could get called simultaneously which
lhotari commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1887675087
> After we make `sendAddSuccessCallbacks` is called by the same thread, we
don't need synchronize.
@hangc0276 True, but that would be a significant change that could at least
lhotari commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1887638998
> The `changingEnsemble` is already protected by synchronized, why do we
need volatile?
@hangc0276 `changingEnsemble` is mutated under `synchronized (metadataLock)
{`. A
This is an automated email from the ASF dual-hosted git repository.
mmerli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 113d40ac50 Change callbacks in
merlimat merged PR #4123:
URL: https://github.com/apache/bookkeeper/pull/4123
--
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:
hangc0276 commented on code in PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#discussion_r1449034362
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##
@@ -1811,12 +1813,17 @@ void sendAddSuccessCallbacks() {
// entries
hangc0276 commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1887421583
@lhotari
>make changingEnsemble field volatile
The `changingEnsemble` is already protected by synchronized, why do we need
volatile?
> change line 204 in
lhotari commented on code in PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#discussion_r1448682604
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##
@@ -1811,12 +1813,17 @@ void sendAddSuccessCallbacks() {
// entries
lhotari commented on code in PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#discussion_r1448682604
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##
@@ -1811,12 +1813,17 @@ void sendAddSuccessCallbacks() {
// entries
lhotari commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1886869318
@graysonzeng After spending more time with this issue, here are my
suggestions to fix the issue:
* make `sendAddSuccessCallbacks` method `synchronized`.
* make
eolivelli commented on code in PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#discussion_r1448540352
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##
@@ -1811,12 +1813,17 @@ void sendAddSuccessCallbacks() {
// entries
eolivelli commented on code in PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#discussion_r1448521452
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##
@@ -1811,12 +1813,17 @@ void sendAddSuccessCallbacks() {
// entries
graysonzeng commented on PR #4171:
URL: https://github.com/apache/bookkeeper/pull/4171#issuecomment-1886647231
> The logic in `sendAddSuccessCallbacks` in master branch feels wrong. The
logic gets stuck if the call to sendAddSuccessCallbacks is missed or out of
order. Calling the method
18 matches
Mail list logo
