rdhabalia commented on code in PR #22846:
URL: https://github.com/apache/pulsar/pull/22846#discussion_r1628220942
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java:
##########
@@ -116,25 +115,32 @@ public synchronized void setConf(Configuration conf) {
super.setConf(conf);
MetadataStore store;
try {
- store = createMetadataStore(conf);
- bookieMappingCache =
store.getMetadataCache(BookiesRackConfiguration.class);
- store.registerListener(this::handleUpdates);
- racksWithHost = bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH).get()
- .orElseGet(BookiesRackConfiguration::new);
- for (Map<String, BookieInfo> bookieMapping :
racksWithHost.values()) {
- for (String address : bookieMapping.keySet()) {
- bookieAddressListLastTime.add(BookieId.parse(address));
- }
- if (LOG.isDebugEnabled()) {
- LOG.debug("BookieRackAffinityMapping init,
bookieAddressListLastTime {}",
- bookieAddressListLastTime);
- }
- }
- updateRacksWithHost(racksWithHost);
- watchAvailableBookies();
- } catch (InterruptedException | ExecutionException | MetadataException
e) {
+ store = getMetadataStore(conf);
+ } catch (MetadataException e) {
throw new RuntimeException(METADATA_STORE_INSTANCE + " failed to
init BookieId list");
}
+
+ bookieMappingCache =
store.getMetadataCache(BookiesRackConfiguration.class);
+ store.registerListener(this::handleUpdates);
+ bookieMappingCache.get(BOOKIE_INFO_ROOT_PATH)
Review 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?
--
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]
