-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Client should track current broker leader #1877
Conversation
This may not be a complete implementation, but should be an improvement. |
Looks good, can you add a test? |
14a6382
to
e7faeee
Compare
Cool, let me look into a test now. |
e7faeee
to
63aef03
Compare
@benbjohnson -- can you take another look? I added tests, and also fixed a bug -- the entire redirect URL was getting set in the client, when it should only have been the host (and port if present). |
2a77f45
to
6983c4c
Compare
return | ||
} | ||
|
||
v, err := url.Parse(u.Scheme + "://" + u.Host) |
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.
It's way easier to just create a new url.URL
:
v := &url.URL{Scheme: u.Scheme, Host: u.Host}
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.
Got it -- I felt there had to be a better way.
One comment on the URL copy but otherwise lgtm. |
Thanks for the feedback @benbjohnson -- made that one final change. I will merge on green. |
4b88c3d
to
fa4798e
Compare
fa4798e
to
9a6f74c
Compare
Green build, merging now. |
Client should track current broker leader
No description provided.