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.

Sign up for GitHub

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

Jump to bottom

SOLR-17019: ZkCli should create subpaths when necessary #1998

Merged
merged 1 commit into from
Oct 11, 2023
Merged

SOLR-17019: ZkCli should create subpaths when necessary #1998

merged 1 commit into from
Oct 11, 2023

Conversation

HoustonPutman
Copy link
Contributor

@HoustonPutman HoustonPutman marked this pull request as ready for review October 10, 2023 19:20
risdenk
risdenk approved these changes Oct 10, 2023
View reviewed changes
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.

Do we need to worry about ACLs on these created znodes?

@HoustonPutman
Copy link
Contributor Author

Do we need to worry about ACLs on these created znodes?

Good question! So zkCli.sh take the SOLR_ZK_CREDS_AND_ACLS envVar, just like the bin/solr tool commands do. So both should use the same ACLs when starting the Solr ZK Client.

stillalex
stillalex reviewed Oct 10, 2023
View reviewed changes
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ClusterProperties.java
@@ -220,7 +220,7 @@ public void setClusterProperty(String propertyName, Object propertyValue) throws
@SuppressWarnings({"rawtypes"})
Map properties = new LinkedHashMap();
properties.put(propertyName, propertyValue);
client.create(
client.makePath(
Copy link
Member

Choose a reason for hiding this comment

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

minor: this doesn't seem to need the change, given that it's a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what necessitated the change. the chroot that the user is using might not exist yet.

Copy link
Member

Choose a reason for hiding this comment

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

yep. wrote the comment and like 10 mins later...oh yeah... thanks for confirming :)

@epugh
Copy link
Contributor

epugh commented Oct 10, 2023

I don't know if this matters at all, but at some point, I'd like to see what zkCli.sh does be subsumed into bin/solr zk commands. Obviously with the appropriate deprecation process...

@HoustonPutman
Copy link
Contributor Author

I don't know if this matters at all, but at some point, I'd like to see what zkCli.sh does be subsumed into bin/solr zk commands. Obviously with the appropriate deprecation process...

Yeah, I'm not a fan of the separate utilities. One path for all this stuff would be much better. Anyways, this would need to be fixed either way.

@HoustonPutman HoustonPutman merged commit 3ab1683 into apache:main Oct 11, 2023
@HoustonPutman HoustonPutman deleted the zk-cli-path branch October 11, 2023 01:17
HoustonPutman added a commit that referenced this pull request Oct 11, 2023
@HoustonPutman
SOLR-17019: ZkCli should create subpaths when necessary (#1998)
884c32e 
(cherry picked from commit 3ab1683)
HoustonPutman added a commit that referenced this pull request Oct 11, 2023
@HoustonPutman
SOLR-17019: ZkCli should create subpaths when necessary (#1998)
71e101b 
(cherry picked from commit 3ab1683)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stillalex stillalex stillalex approved these changes

@risdenk risdenk risdenk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HoustonPutman @epugh @stillalex @risdenk