sijie commented on issue #1192: BP-29 (task 3): use metadata service uri for constructing registration client URL: https://github.com/apache/bookkeeper/pull/1192#issuecomment-369003391 > It's replacing it as far as BookKeeper is concerned. BookKeeper only has RegClient to manage the ownership of regClient. This is now handed off to MetadataClientDriver. this doesn't make the call. bookkeeper is keeping this reference for simplifying the accesses to this class, just like bookkeeper/ledger handles keep references to counters/op stats loggers. at your initial comment: `In the latest patch, it is only ever referenced a couple of lines down in the constructor`, which is not entirely true for `regClient`. you only see the places you need to change when you actually do the code change and I did the code change and I know the places I need to update in order to compromise removing `regClient` and that's why I didn't remove it. but anyway, since you feel really strong on this. I don't want to be stuck here. I changed all the places to remove `regClient` as you requested. > My problem with this is that allowOverride is brought into the default flow of the method. It's even the first branch from the if on L183. It means multiple returns from the method. Logging only occurs if there is a race. The existence in the map is checked twice. And what should be the non-default path is intertwined with the default. None of these are huge in themselves, but they combine to make me dislike the current flow a lot. Another alternative, which would be a least deal with some of the intertwining... no strong preference on how things should be implemented here. since you have a strong preference on branches, I would change to what you suggested.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
