[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2788: fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver

[GitBox <gi...@apache.org>]

dlg99 commented on a change in pull request #2788:
URL: https://github.com/apache/bookkeeper/pull/2788#discussion_r704577640



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
##########
@@ -256,10 +256,12 @@ public RackawareEnsemblePlacementPolicyImpl initialize(ClientConfiguration conf,
         DNSToSwitchMapping dnsResolver;
         if (optionalDnsResolver.isPresent()) {
             dnsResolver = optionalDnsResolver.get();
+            dnsResolver.setBookieAddressResolver(bookieAddressResolver);

Review comment:
       I'd assume that provided `optionalDnsResolver` should be ready to use / have the `bookieAddressResolver` set, as it happens there:
   https://github.com/apache/bookkeeper/blob/97818f5123999396e66f5246420d3c7e3d25f53d/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java#L500-L507
   
   Have you traced why we happen to have the optional dns resolver provided here yet not properly configured?
   I think the right place to fix the issue is where the dns resolver is created and passed to the dnsResolver.
   Shall we move bookieAddressResolver into  DNSToSwitchMapping constructor (or Builder?) and make it mandatory (preconditions check)?




-- 
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