-
Notifications
You must be signed in to change notification settings - Fork 472
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
fix: add more check in cluster node parsing #2538
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test case for this patch?
Test case is added + the node checking is more flexible |
@PragmaTwice Are you able to see what exactly is causing the fail on the error, I don't seem to be able to see the reason for it. |
@jonathanc-n #2401 is caused by missing a newline between nodes, and omitting the newline at the end should be allowed. |
@git-hulk Alright, it should be fully working now. |
@jonathanc-n You can enhance this inside parseClusterNodes |
@PragmaTwice @git-hulk This should be good I believe, I moved the changes to cluster.cc. |
@jonathanc-n It would be better to check if the field is node id in line 778? kvrocks/src/cluster/cluster.cc Line 778 in 2a0c57a
The current solution is too tricky from my perspective. |
@git-hulk I simplified as much as I thought I could for the issue to not persist. I believe it should be fine. |
@git-hulk @PragmaTwice Pretty sure this should be good, it passed the workflow. Unless you see some changes that should be made. |
Co-authored-by: hulk <hulk.website@gmail.com>
@git-hulk Alright, should be fine now, sorry about that. |
@jonathanc-n No worry, thanks for your great contribution. |
Quality Gate passedIssues Measures |
references #2401
Added non confusing message when multiple nodes are added without a newline separator