[GitHub] [pulsar] horsteff commented on a change in pull request #7533: [Issue 7492][pulsar-broker] Cleanup configuration process when using PulsarStandaloneBuilder

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

horsteff commented on a change in pull request #7533:
URL: https://github.com/apache/pulsar/pull/7533#discussion_r522523167



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandaloneBuilder.java
##########
@@ -37,8 +48,8 @@ public static PulsarStandaloneBuilder instance() {
         return new PulsarStandaloneBuilder();
     }
 
-    public PulsarStandaloneBuilder withConfig(ServiceConfiguration config) {

Review comment:
       Normally I would agree. But in this case the method `withConfig` doesn't do anything useful at all as the given value will be simply ignored. It is overwritten at start of the `build` method with a freshly created `ServiceConfiguration` instance. At first I tried to fix the build code and keep the method, but I didn't see a useful way to do this without rewriting the complete start process of `PulsarStandalone`. And if someone wants to change configuration properties by code, they can do it after calling `build` and before starting the `PulsarStandalone` instance (may be they've done it already as it's the only way it was working). So it even was not worth the effort.
   
   I have my doubts, that `PulsarStandaloneBuilder` is already much in use, so backward compatibility shouldn't be a big issue. But because `withConfig` does practically nothing, I think it's better to force those, who still use this method (erroneously expecting the given configuration will be used), to rethink their code by removing this method. And for new users this method would be a source of annoyance, if they try to use it and have to discover, that it does not work, and possibly file a new bug report which isn't easy to fix as I said above. So I would say in this case braking backward compatibility and removing the method is the better option.
   




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org