[ISSUE #8] Offer a more user-friendly format to configure nameserver#171
[ISSUE #8] Offer a more user-friendly format to configure nameserver#171RongtongJin wants to merge 3 commits intoapache:masterfrom
Conversation
| } else { | ||
| nameServerToString = nameServer.get(0); | ||
| if (nameServerToString.contains(";")) { | ||
| log.warn("name-server format `host:port;host:port` is deprecated."); |
There was a problem hiding this comment.
Should have a recommendation for a newer style.
There was a problem hiding this comment.
IMO that logic shows a separate property is warranted.
snicoll
left a comment
snicoll
left a comment
There was a problem hiding this comment.
I don't think changing the cardinality of the current property is a good idea. I've added a suggestion.
| * The name servers for rocketMQ, formats: `host:port,host:port`. | ||
| */ | ||
| private String nameServer; | ||
| private List<String> nameServer; |
There was a problem hiding this comment.
The name seems a bit odd for a collections. Have you considered naming that servers ? If you want to retain backward compatibility, you could deprecate this one with a replacement metadata to servers and then apply the old logic if nameServer is set. If both are set, I would throw an exception.
You could then remove nameServer in the next feature release.
| } else { | ||
| nameServerToString = nameServer.get(0); | ||
| if (nameServerToString.contains(";")) { | ||
| log.warn("name-server format `host:port;host:port` is deprecated."); |
There was a problem hiding this comment.
IMO that logic shows a separate property is warranted.
|
I will close this PR. Considering the habit of rocketmq users, I think separate namesever with semicolons is still a good choice. |
|
@RongtongJin I am confused. This issue is closed in |
|
@snicoll, I apologize for not replying you sooner, I missed your message since I closed this pull request. I think this is a historical problem. Rocketmq users have used the |
|
Great, let's hear more from the community. |
3 participants
What is the purpose of the change
User can config name-server with format
host:port,host:portor other list formats and ensure backward compatibility.ref #8
Brief changelog
Change the injection type from string to List.
Verifying this change
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyleto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.