Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RocketMQProperties#nameServer should offer a more user-friendly format #8

Open
snicoll opened this issue Dec 14, 2018 · 9 comments
Open
Labels
enhancement New feature or request

Comments

@snicoll
Copy link
Contributor

snicoll commented Dec 14, 2018

The nameServer property ultimately maps to ClientConfig#setNamesrvAddr that requires a host:port pair separated by ;

I think it would be nicer to not expose that complexity back to the configuration. Rather, the configuration should expose a List<String> where each item represent a host:port pair.

There are several advantages to this:

  • Each pair can be checked individually and an exception can be thrown if its format is invalid
  • YAML configuration is much more readable as you can use the native list format
  • Perhaps the property can be renamed to servers to express it is a list

Taking this into consideration (+ #9) the property can be configured as follows:

rocketmq:
  servers:
    - 10.0.0.1:9876
    - 10.0.0.2:9876
@vongosling vongosling added the enhancement New feature or request label Dec 17, 2018
@vongosling vongosling added this to the 2.0.1 milestone Dec 17, 2018
@walking98
Copy link
Contributor

This is a good suggestion, at present most of the RocketMQ users get used to define multiple name-servers with the single line property. I incline to defer this enhancement.

@vongosling
Copy link
Member

@snicoll Could you help to review and find out the remaining problems or trivial polish, we would release the 2.0.1, hoping including all improvements.

@vongosling vongosling modified the milestones: 2.0.1, 2.0.2 Dec 20, 2018
@snicoll
Copy link
Contributor Author

snicoll commented Dec 20, 2018

@vongosling are you asking in the context of this particular issue or globally? If you're asking globally, you can reach out by email (available on my profile). If you're asking in the context of this issue, I am afraid I didn't get the request.

@vongosling
Copy link
Member

@snicoll Hah, I wonder could you have any other comments in rocketmq-spring-boot-starter. I am not sure whether your colleague aclement request you to help the community review the code. Anyway, I very appreciate your guys' review help :-)

@snicoll
Copy link
Contributor Author

snicoll commented Dec 20, 2018

Got it. Yes, I am working with Andy. I'll do one more pass today.

@vongosling vongosling modified the milestones: 2.0.2, 2.0.3 Apr 3, 2019
@ShannonDing ShannonDing modified the milestones: 2.0.3, 2.0.4 May 24, 2019
@snicoll
Copy link
Contributor Author

snicoll commented Nov 11, 2019

May I ask why this issue was closed? I don't see any change on master that relates to this request.

@RongtongJin
Copy link
Contributor

Hi @snicoll, you can reopen this issue, I will submit a PR to slove this issue and ensure backward compatibility.

@snicoll
Copy link
Contributor Author

snicoll commented Nov 13, 2019

Thank you but I don’t have the rights to reopen it.

@vongosling
Copy link
Member

@snicoll Could you help to review this improvement? YAML and list style(rocketmq.name-server[0]=XXX, rocketmq.name-server[1]=XXX. Or rocketmq.name-server= XXX) are all supported and vefified now.

@vongosling vongosling added this to the 2.1.0 milestone Nov 15, 2019
@RongtongJin RongtongJin modified the milestones: 2.1.0, 2.1.1 Feb 10, 2020
@RongtongJin RongtongJin removed this from the 2.1.1 milestone Jul 16, 2020
vongosling pushed a commit that referenced this issue Sep 8, 2021
merge from rocketmq spring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants