[fix] Remove blocking calls from BookieRackAffinityMapping #22846
Conversation
merlimat
requested review from
lhotari,
rdhabalia,
hangc0276,
codelipenghui,
horizonzy and
zymap
|
|
||
| bookieMappingCache = store.getMetadataCache(BookiesRackConfiguration.class); | ||
| store.registerListener(this::handleUpdates); | ||
| bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH) |
There was a problem hiding this comment.
this async processing may cause a race condition and bk-client may create ledgers before it updates bookie with rack info, and that can cause ledgers to be created on the wrong bookies and that will not work for us because we need strict bookie affinity and we can't afford ledgers created on wrong bookies.
There was a problem hiding this comment.
valid point. I guess this is hard to solve since the interface is synchronous in Bookkeeper. I guess there are ways to workaround the problem and still make it asynchronous. In that case, the initialization would have to somehow happen before the bk-client is made ready.
There was a problem hiding this comment.
Ok, makes sense. I think we should still change the sync call in the watcher event. It's unlikely to have a cache miss there, though it's technically possible.
Without changing BK client constructor to reflect the blocking operation that happens, I think we could change the BK provider class to do the creation in background.
There was a problem hiding this comment.
I think it does not solve the problem completely because the calling thread is still blocked.
The BK client creation is moved to a background thread and it's ok, thought the thread waiting for the BK client to be created is still blocked.
eg:
- IO thread is initiating the creation of topic
- We trigger creation of BK client
- background-thread is creating BK client
- IO thread is still blocked waiting for completion.
I'm working on a change to return future from BK client factory that would avoid the problem.
There was a problem hiding this comment.
merging this for now since it's solves a (small) part of the problem
There was a problem hiding this comment.
The BK client creation is moved to a background thread and it's ok, thought the thread waiting for the BK client to be created is still blocked.
No, this solves the issue. deadlock was happening at org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl.asyncOpen at ledgers.computeIfAbsent
Moving bk-client creation async will not hold the lock at concurrentHashMap and that will also not cause metadata-thread waiting.
infact this PR should not be merged. Making change in bk-client and releasing bk version takes time and not all previous version can take that change. so, we need a fix which can solve the problem and #22842 should do it.
so, if there is no issue with #22842 then can we merge it?
There was a problem hiding this comment.
infact this PR should not be merged.
This is solving a problem your PR was not addressing, the use of blocking call in the notification watcher.
(cherry picked from commit aece67e)
|
related stack |
) (cherry picked from commit aece67e)
…pache#22846)" This reverts commit aece67e.
|
I encounter rack information lost issue with this pr's modification, and do an analysis. Can you take a look? @merlimat @rdhabalia I think we can not make the watch async. |
Motivation
There are a couple of blocking calls to metadata in
BookieRackAffinityMappingclass, in places where we're not supposed to make any blocking calls.This is made a bit hard by the BK rack-affinity interface that doesn't allow to use futures.
Modifications
During the initial
setConf()load the list of bookie-racks info asynchronously, and set up the async watch. This seems the most sensible approach, barring a complete redesign of the rack-affinity interface in BK.Documentation
docdoc-requireddoc-not-neededdoc-complete