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

SOLR-16150 Provide correct bound zookeeper interface #802

Merged
merged 6 commits into from Oct 18, 2022

Conversation

madrob
Copy link
Contributor

@madrob madrob commented Apr 11, 2022

Instead of always giving localhost, we can return the interface that ZK
actually listens on.

https://issues.apache.org/jira/browse/SOLR-16150

Instead of always giving localhost, we can return the interface that ZK
actually listens on.
Copy link
Contributor

@anshumg anshumg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -70,7 +70,7 @@ public String getClientString() {

// if the string wasn't passed as zkHost, then use the standalone server we started
if (zkRun == null) return null;
return "localhost:" + zkProps.getClientPortAddress().getPort();
return zkProps.getClientPortAddress().getHostString() + ":" + zkProps.getClientPortAddress().getPort();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I was trying to see if we need to switch to using zkProps.getSecureClientPortAddress optionally but couldn't find anything in the ZK javadocs. Also, based on my ZK TLS testing - didn't seem like that was needed.

@epugh
Copy link
Contributor

epugh commented Apr 12, 2022

This is great, I've seen this noise output and always find it "alarming" to see!

@madrob
Copy link
Contributor Author

madrob commented Jun 10, 2022

This needs to be tested with docker containers before merging.

Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was good to go and then stalled. Would be good to get this in :) looks like it only needs updating to main.

@risdenk risdenk self-assigned this Oct 17, 2022
@risdenk risdenk merged commit c80cf5b into apache:main Oct 18, 2022
risdenk added a commit that referenced this pull request Oct 19, 2022
Instead of always giving localhost, we can return the interface that ZK actually listens on.

Co-authored-by: Kevin Risden <krisden@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants