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
cause performance regressions. The Bookkeeper client code doesn't seem to use
the OrderedExecutor in most cases. The current code base is using synchronized
in this area.
After thinking about it, it might not be necessary to make the method itself
`synchronized`. It might not even be necessary to run it with an
OrderedExecutor. I think that it's sufficient to ensure that
sendAddSuccessCallbacks and drainPendingAddsAndAdjustLength methods don't race
and are serially executed. A simple `synchronized(pendingAddOps) {` could be
the simplest possible solution. It's also possible that a race between
sendAddSuccessCallbacks and drainPendingAddsAndAdjustLength methods isn't even
a problem.
> IMO, the drainPendingAddsAndAdjustLength method also need to be executed
by OrderedExecutor.
The issue in drainPendingAddsAndAdjustLength isn't an ordering problem. The
method simply doesn't update pendingAddsSequenceHead so that
sendAddSuccessCallbacks could work after drainPendingAddsAndAdjustLength has
been called. The instance will get stuck when pendingAddsSequenceHead gets out
of sync in drainPendingAddsAndAdjustLength. My current assumption is that it's
the root cause of this issue together with the changingEnsemble thread
visibility issue.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
