You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/07/15 15:59:43 UTC
[GitHub] [solr] haythemkh opened a new pull request, #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
haythemkh opened a new pull request, #943:
URL: https://github.com/apache/solr/pull/943
https://issues.apache.org/jira/browse/SOLR-15342
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
I open this PR in draft to share the evolution of the work. I closed the old [PR](https://github.com/apache/solr/pull/584) since it has been open for a while, there have been quite a few changes since then in relation to ZooKeeper and some of the work has been contributed in a separate [PR](https://github.com/apache/solr/pull/749). I am wearing the same changes made in this last one.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r947297013
##########
solr/solrj/build.gradle:
##########
@@ -50,20 +41,15 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
- exclude group: "org.apache.yetus", module: "audience-annotations"
- exclude group: "log4j", module: "log4j"
- exclude group: "org.slf4j", module: "slf4j-log4j12"
- })
- implementation ('org.apache.zookeeper:zookeeper-jute') {
- exclude group: 'org.apache.yetus', module: 'audience-annotations'
- }
-
testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
testImplementation project(':solr:solrj')
testRuntimeOnly project(':solr:modules:sql')
+ // ideally ZK centric tests move to solrj-zookeeper but sometimes we depend on ZK here anyway
Review Comment:
Umm; I don't care enough any way. I did call it out in the PR conversation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r930468981
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/util/ZkUtils.java:
##########
@@ -32,4 +40,45 @@ public static Map<String, Object> getJson(
}
return Collections.emptyMap();
}
+
+ @SuppressWarnings({"unchecked"})
+ public static Map<String, Object> getJson(DistribStateManager distribStateManager, String path)
Review Comment:
You can move this method to be a method directly on DistribStateManager, thus not needing to take distribStateManager as an argument, as it'd be "this".
##########
solr/test-framework/src/java/org/apache/solr/cloud/CloudSolrClientUtils.java:
##########
@@ -0,0 +1,109 @@
+package org.apache.solr.cloud;
+
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.common.cloud.CollectionStatePredicate;
+import org.apache.solr.common.cloud.CollectionStateWatcher;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.DocCollectionWatcher;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+
+import org.apache.solr.common.cloud.ZkStateReader;
+
+public class CloudSolrClientUtils {
Review Comment:
These methods are all not-needed I think, thus we don't need this utility class holder either. First, observe that "connect" effectively does nothing for the ZK implementation. Thus these methods (waitForState, ...) here that call it and then immediately call assertZKStateProvider mean that calling connect() first is pointless. These methods are effectively one-liners at that point and you can inline them. IntelliJ makes this trial work via the "inline" refactoring.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/StatementImpl.java:
##########
@@ -76,9 +75,8 @@ private ResultSet executeQueryImpl(String sql) throws SQLException {
protected SolrStream constructStream(String sql) throws IOException {
try {
- ZkStateReader zkStateReader = ZkStateReader.from(this.connection.getClient());
Slice[] slices =
- CloudSolrStream.getSlices(this.connection.getCollection(), zkStateReader, true);
+ CloudSolrStream.getSlices(this.connection.getCollection(), this.connection.getClient(), true);
Review Comment:
This particular commit is nice; it will allow a SolrJ client using streaming expressions to not require ZK needlessly. At least it's a step in that direction if not totally there. CC @joel-bernstein
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -700,25 +675,6 @@ public static SpecProvider getSpec(final String name) {
};
}
- public static String parseMetricsReplicaName(String collectionName, String coreName) {
Review Comment:
Looks like you moved this and realized you didn't need to. But when it came back; it's in a different spot in this class.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -128,7 +130,7 @@ public DocCollection(
}
this.activeSlicesArr = activeSlices.values().toArray(new Slice[activeSlices.size()]);
this.router = router;
- this.znode = ZkStateReader.getCollectionPath(name);
+ this.znode = "/collections/" + name + "/state.json"; // nocommit : check if this needs to become generic
Review Comment:
LGTM
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/util/ZkUtils.java:
##########
@@ -0,0 +1,35 @@
+package org.apache.solr.common.util;
+
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkOperation;
+import org.apache.zookeeper.KeeperException;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class ZkUtils {
Review Comment:
Don't create another utility class. In this case, the method is used in exactly one spot and so you can inline it.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -67,7 +67,51 @@ public ZkClientClusterStateProvider(String zkHost) {
this.zkHost = zkHost;
}
- @Override
+ /**
+ * Create a ClusterState from Json. This method supports legacy configName location
+ *
+ * @param bytes a byte array of a Json representation of a mapping from collection name to the
+ * Json representation of a {@link DocCollection} as written by {@link ClusterState#write(JSONWriter)}. It
+ * can represent one or more collections.
+ * @param liveNodes list of live nodes
+ * @param coll collection name
+ * @param zkClient ZK client
+ * @return the ClusterState
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonSupportingLegacyConfigName(
Review Comment:
@HoustonPutman I observed that in #749 you put this on ZkStateReader. Is ZkClientclusterStateProvider fine? I don't have a strong opinion.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkCmdExecutor.java:
##########
@@ -44,7 +43,7 @@ public ZkCmdExecutor(int timeoutms) {
* class.
*/
public ZkCmdExecutor(int timeoutms, IsClosed isClosed) {
- timeouts = timeoutms / 1000.0;
+ double timeouts = timeoutms / 1000.0;
Review Comment:
not a big deal but please refrain from making little changes like this (probably suggested by your IDE) unless you need to touch a piece of code any way. (moving a class doesn't count)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/DatabaseMetaDataImpl.java:
##########
@@ -118,7 +118,7 @@ public String getDatabaseProductVersion() throws SQLException {
SolrClient solrClient = null;
for (String node : liveNodes) {
try {
- String nodeURL = ZkStateReader.from(cloudSolrClient).getBaseUrlForNodeName(node);
+ String nodeURL = Utils.getBaseUrlForNodeName(node, /*getClusterProperty(URL_SCHEME, "http")*/ "http");
Review Comment:
That's "nocommit" worthy :-)
I suppose CloudSolrClient.getClusterStateProvider should have a getBaseUrlForNodeName() instance method. The ZK implementation could call the one on ZkStateReader so that this scheme is consulted.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -293,7 +293,7 @@ public static Slice[] getSlices(
// check for alias or collection
- List<String> allCollections = new ArrayList<>();
+ List<String> allCollections = new ArrayLigist<>();
Review Comment:
Please compile and tidy before committing & pushing :-)
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/util/ZkUtils.java:
##########
@@ -32,4 +40,45 @@ public static Map<String, Object> getJson(
}
return Collections.emptyMap();
}
+
+ @SuppressWarnings({"unchecked"})
+ public static Map<String, Object> getJson(DistribStateManager distribStateManager, String path)
+ throws InterruptedException, IOException, KeeperException {
+ VersionedData data;
+ try {
+ data = distribStateManager.getData(path);
+ } catch (KeeperException.NoNodeException | NoSuchElementException e) {
+ return Collections.emptyMap();
+ }
+ if (data == null || data.getData() == null || data.getData().length == 0)
+ return Collections.emptyMap();
+ return (Map<String, Object>) Utils.fromJSON(data.getData());
+ }
+
+ public static InputStream toJavabin(Object o) throws IOException {
Review Comment:
Move to a utility method on JavaBinCodec.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -282,50 +277,6 @@ public List<TupleStream> children() {
return solrStreams;
}
- public static Slice[] getSlices(
Review Comment:
Where did this method go?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java:
##########
@@ -347,14 +346,14 @@ private StreamComparator parseComp(String sort, String fl) throws IOException {
}
public static Slice[] getSlices(
Review Comment:
@joel-bernstein We are changing the API signature. Is this fine to bring to Solr 8 -- e.g. is it obscure? If it's a problem, this change would not occur and instead the CloudSolrStream stuff would need to go to the SolrJ-Zookeeper module. I suppose we should just do that. Not an action to take now but something to remember in the backport.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java:
##########
@@ -43,6 +43,9 @@
public class Slice extends ZkNodeProps implements Iterable<Replica> {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // nocommit : to move this from ZkStateReader
+ private static final String STATE_PROP = "state";
Review Comment:
This is a good new home.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -47,6 +41,14 @@
public class DocCollection extends ZkNodeProps implements Iterable<Slice> {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // nocommit : to move these from ZkStateReader
Review Comment:
I think this is a good home for these properties now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952744870
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Solrj-ZooKeeper - SolrJ requiring ZooKeeper'
+
+dependencies {
+ // Spotbugs Annotations are only needed for old findbugs
+ // annotation usage like in Zookeeper during compilation time.
+ // It is not included in the release so exclude from checks.
+ compileOnly 'com.github.spotbugs:spotbugs-annotations'
+ permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
+ // Exclude these from jar validation and license checks.
+ configurations.jarValidation {
+ exclude group: "com.github.spotbugs", module: "spotbugs-annotations"
+ }
+
+ implementation project(':solr:solrj')
+
+ // declare dependencies we use even though already declared by solrj-core
+ implementation 'org.slf4j:slf4j-api'
+ implementation 'org.apache.httpcomponents:httpclient'
+ implementation 'org.apache.httpcomponents:httpcore'
+
+ implementation('org.apache.zookeeper:zookeeper', {
+ exclude group: "org.apache.yetus", module: "audience-annotations"
+ exclude group: "log4j", module: "log4j"
+ exclude group: "org.slf4j", module: "slf4j-log4j12"
+ })
+ implementation ('org.apache.zookeeper:zookeeper-jute') {
+ exclude group: 'org.apache.yetus', module: 'audience-annotations'
+ }
+
+ testImplementation project(':solr:test-framework')
+ testImplementation project(':solr:core')
+
+ testImplementation 'junit:junit'
+ testImplementation 'commons-io:commons-io'
+ testImplementation 'com.google.guava:guava'
+
+ permitTestUsedUndeclared project(':solr:solrj-zookeeper') // duh!
Review Comment:
We don't use `permitTestUsedUndeclared` anywhere else so something is probably wired up weird here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] haythemkh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
haythemkh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r930593883
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -293,7 +293,7 @@ public static Slice[] getSlices(
// check for alias or collection
- List<String> allCollections = new ArrayList<>();
+ List<String> allCollections = new ArrayLigist<>();
Review Comment:
This is already fixed in the PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1199778111
> oops, was trying to be helpful resolving the merge conflict but forgot to compile locally before pushing, sorry. will try to fix, or else commit reversion logically should also be possible? sorry for the noise.
added 3 commits to make it compile a little more but not fully. not sure if it was compiling before actually. again, sorry for the noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952985859
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
Note this particular choice was established in SOLR-16061 in March. Renaming this method would be a separate issue. `extractFrom` seems fine; perhaps even shorter, just `get`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1185790089
Ok I will do it following this getting merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952899077
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
The method name does make it sound like it's possibly creating a new instance. Maybe renaming the method to `extractFrom` could be helpful? Not a deal breaker, just could be a bit easier to understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952750468
##########
gradle/documentation/render-javadoc.gradle:
##########
@@ -325,6 +342,7 @@ class RenderJavadocTask extends DefaultTask {
// lucene-test-framework was first, or broken links to things like LuceneTestCase if lucene-core was first)
if (project.path != ':solr:test-framework') { //
findRenderTasksInDependencies()
+ .findAll({it.project.path != ':solr:solrj' && it.project.path != ':solr:solrj-zookeeper'}) // split packages
Review Comment:
Why is this needed? I know it mentions split packages - but don't we have split packages elsewhere and those modules aren't listed here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952737321
##########
solr/solrj/build.gradle:
##########
@@ -50,20 +41,16 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
- exclude group: "org.apache.yetus", module: "audience-annotations"
- exclude group: "log4j", module: "log4j"
- exclude group: "org.slf4j", module: "slf4j-log4j12"
- })
- implementation ('org.apache.zookeeper:zookeeper-jute') {
- exclude group: 'org.apache.yetus', module: 'audience-annotations'
- }
-
testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
testImplementation project(':solr:solrj')
testRuntimeOnly project(':solr:modules:sql')
+ // ideally ZK centric tests move to solrj-zookeeper but sometimes we depend on ZK here anyway
+ testImplementation project(':solr:solrj-zookeeper')
+ testImplementation 'org.apache.zookeeper:zookeeper'
+ permitTestUnusedDeclared 'org.apache.zookeeper:zookeeper'
Review Comment:
Hmmm thats interesting - we have very very few of these in the code already.
```
➜ solr git:(SOLR-16187) git grep -F Unused | grep -F build.gradle
solr/core/build.gradle: permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
solr/modules/hadoop-auth/build.gradle: permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
solr/modules/hadoop-auth/build.gradle: permitUnusedDeclared 'org.apache.hadoop:hadoop-annotations'
solr/modules/hdfs/build.gradle: permitTestUnusedDeclared 'org.apache.zookeeper:zookeeper'
solr/modules/jwt-auth/build.gradle: //permitTestUnusedDeclared 'com.fasterxml.jackson.core:jackson-databind'
solr/modules/sql/build.gradle: permitUnusedDeclared 'org.apache.calcite.avatica:avatica-core'
solr/solrj/build.gradle: permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
solr/test-framework/build.gradle: permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
solr/test-framework/build.gradle: permitUnusedDeclared 'commons-cli:commons-cli'
solr/webapp/build.gradle: permitUnusedDeclared project(":solr:core")
```
Most of these are spotbugs annotations. I know there were bugs in older versions of the plugin. The hdfs module does have the same thing for zookeeper. So I'm not really sure what is going on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952899077
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
The method name does make it sound like it's possibly creating a new instance. Maybe renaming the method to `extractFrom` could be helpful? Not a deal breaker, just could be a bit easier to understand.
To be clear, this is out of scope for this ticket, was just thinking "out loud".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r947196520
##########
solr/solrj/build.gradle:
##########
@@ -50,20 +41,15 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
- exclude group: "org.apache.yetus", module: "audience-annotations"
- exclude group: "log4j", module: "log4j"
- exclude group: "org.slf4j", module: "slf4j-log4j12"
- })
- implementation ('org.apache.zookeeper:zookeeper-jute') {
- exclude group: 'org.apache.yetus', module: 'audience-annotations'
- }
-
testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
testImplementation project(':solr:solrj')
testRuntimeOnly project(':solr:modules:sql')
+ // ideally ZK centric tests move to solrj-zookeeper but sometimes we depend on ZK here anyway
Review Comment:
Worth opening a follow up JIRA to track this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1217880794
Looks like the content about `CloudSolrClient` in the ref guide (https://solr.apache.org/guide/solr/latest/deployment-guide/solrj.html#building-and-running-solrj-applications) refers to the use of the solr url for confiugration I wonder if we should tweak the text thgouh to stress that using ZK is a vulnerability? Similar to what you put in the upgrade notes?
Also, do we need to mention the need for `solrj-zookeeper` dependency for `CloudSolrClient` users?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1216568431
At the risk of bikeshedding.... How about `ReplicaStateProps.LEADER` instead of `ReplicaSProps.LEADER` since we don't have an existing concept of `SProps` throughout our codebase?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r954045568
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
Not that SolrClient is already Closable and we should already be taking care to manage their lifecycle. A CloudSolrClient that uses a ZK state provider will close it.
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
Note that SolrClient is already Closable and we should already be taking care to manage their lifecycle. A CloudSolrClient that uses a ZK state provider will close it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1200090588
@haythemkh I know you are using the Google Java Format IntelliJ plugin but not the IntelliJ code style that accompanies it, which ensures that the import wildcards are configured correctly (the plugin can't do that part). See https://github.com/google/google-java-format#intellij-android-studio-and-other-jetbrains-ides
FYI @epugh it would be cool to mention this somewhere for contributor onboarding
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r934454511
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -67,6 +72,50 @@ public ZkClientClusterStateProvider(String zkHost) {
this.zkHost = zkHost;
}
+ /**
+ * Create a ClusterState from Json. This method supports legacy configName location
+ *
+ * @param bytes a byte array of a Json representation of a mapping from collection name to the
+ * Json representation of a {@link DocCollection} as written by {@link
+ * ClusterState#write(JSONWriter)}. It can represent one or more collections.
+ * @param liveNodes list of live nodes
+ * @param coll collection name
+ * @param zkClient ZK client
+ * @return the ClusterState
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonSupportingLegacyConfigName(
+ int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
+ if (bytes == null || bytes.length == 0) {
+ return new ClusterState(liveNodes, Collections.emptyMap());
+ }
+ Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(bytes);
Review Comment:
💬 30 similar findings have been found in this PR
---
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `ZkClientClusterStateProvider.createFromJsonSupportingLegacyConfigName(...)` indirectly writes to field `noggit.JSONParser.devNull.buf` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>Expand here to view all instances of this finding</b></summary><br/>
<div align="center">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java | [299](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java#L299)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [537](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L537)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [1436](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L1436)|
| solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java | [231](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java#L231)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [1447](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L1447)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [1765](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L1765)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [1244](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L1244)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [2108](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L2108)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [569](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L569)|
| solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java | [1914](https://github.com/haythemkh/solr/blob/3efe296542ed1da64d516ebd4152aa3b5441ba31/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L1914)|
<p> Showing <b>10</b> of <b>30</b> findings. <a href="https://lift.sonatype.com/results/github.com/haythemkh/solr/01G9CHRH0BCZM60GSXJQGFR10C?t=Infer|THREAD_SAFETY_VIOLATION" target="_blank">Visit the Lift Web Console</a> to see all.</p></div></details>
---
Reply with *"**@sonatype-lift help**"* for info about LiftBot commands.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get to know more about LiftBot commands.
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=307573340&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=307573340&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=307573340&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=307573340&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=307573340&lift_comment_rating=5) ]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] haythemkh commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
haythemkh commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1201380445
I would love to have your feedback on what we want to do with the nocommits:
- These include copying ZkStateReader constants for solrj classes: [Utils](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/util/Utils.java#L89), [PerReplicaStates](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/cloud/PerReplicaStates.java#L182), [ClusterState](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java#L49), [DocCollection](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java#L44), [Slice](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java#L46), [Replica](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/sol
r/common/cloud/Replica.java#L41), [ZkCoreNodeProps](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/cloud/ZkCoreNodeProps.java#L23), [CloudSolrClient](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java#L93), [BaseHttpClusterStateProvider](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java#L48), [CollectionAdminRequest](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java#L73), [CoreAdminRequest](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/client/solrj/request/CoreAdminRequest.java#L40)
- [Use raw "http" instead of going through #getClusterProperty in DatabaseMetaDataImpl](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/DatabaseMetaDataImpl.java#L123)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952814715
##########
gradle/documentation/render-javadoc.gradle:
##########
@@ -325,6 +342,7 @@ class RenderJavadocTask extends DefaultTask {
// lucene-test-framework was first, or broken links to things like LuceneTestCase if lucene-core was first)
if (project.path != ':solr:test-framework') { //
findRenderTasksInDependencies()
+ .findAll({it.project.path != ':solr:solrj' && it.project.path != ':solr:solrj-zookeeper'}) // split packages
Review Comment:
The link checker of our docs complains about split packages here but perhaps it didn't for other split packages cases? This is possible; it depends on if the dependency reflects itself in the public API. If there is a compile dependency that isn't reflected in the API, then there is no javadoc issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley merged PR #943:
URL: https://github.com/apache/solr/pull/943
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r947362069
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -68,7 +69,21 @@ protected CloudHttp2SolrClient(Builder builder) {
"Both zkHost(s) & solrUrl(s) have been specified. Only specify one.");
}
if (builder.zkHosts != null) {
- this.stateProvider = new ZkClientClusterStateProvider(builder.zkHosts, builder.zkChroot);
+ try {
Review Comment:
I'll add a line -- it's rather fundamental to this PR.
For user level, I added a note to solr-upgrade-notes.adoc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1217331927
Other than a CHANGES.txt entry, I think this PR is ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1217896387
Really looking forward to this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1216138801
For constants related to the collection's state, I put it in an inner interface DocCollection.CollectionSProps. The "S" there is for State. If that's removed, then it's possibly confused with CollectionProperties. It's mostly an internal set of constants (it's not *parameters*) so I think it's okay it it doesn't roll off the tongue. Likewise I put constants for the Replica in Replica.ReplicaSProps. I'm not married to these choices.
There is a CollectionAdminParams class but it was missing quite a number of parameters that CollectionAdminRequest needed to specify so I moved them there.
There were a handful of other constants then I mostly made private and didn't fret over possible redundancy with ZkStateReader.
Overall I like having these props or params as constants in suitable locations, especially when we don't static import these constants. It adds clarity to the spots where they are referred to as being related to parameters or properties or whatever it is.
The only thing that remains is the javadoc split package concern. It should only be a handful of lines of code or so in render-javadoc.adoc to special case them to not inter-link.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1214289465
A big obstacle I'm seeing is split packages between solrj & solrj-zookeeper with respect to javadocs. For example the javadocs for solr-core will now contain broken links to most solrj-zookeeper classes because the javadoc tool can't figure this out (kind of a long-standing deficiency).
Personally, I'm not very sympathetic to the needs of continuing to maintain a javadoc site. I don't use them, I doubt others do, and moreover for open-source, really the source is what we want to see because it's better / more. That's me, anyway. I don't think maintaining them is more important than other goals like modularization, and thus I'm willing for there to be some degradation there.
Options:
* In our javadocs, stop **linking** out to solrj or solrj-zookeeper because they cannot be differentiated by the standard javadoc tool.
* Accept broken links; stop checking for them within javadoc folders.
* Fix it (not something I'm willing to invest in)
Of course in main/10.0 we can remove the split packages but that's a back-combat break that isn't okay for 9.x.
CC @uschindler -- I'm curious what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1219573696
Planned commit message after the first line:
```
• CloudSolrClient builders use reflection to instantiate ZkClientClusterStateProvider when needed
• CollectionAdminParams constants
• CollectionStateProps constants
• SliceStateProps constants
• ReplicaStateProps constants
• SolrJ streaming expressions: don’t depend on ZK anymore
• javadocs: temporarily unlink to solrj modules due to split package issue
• PerReplicaStates.fetch -> PerReplicaStatesFetcher
• ZkStateReader.getCollectionPath -> DocCollection.getCollectionPath
```
Some other code changes seemed too obscure to mention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1185783850
> Once that is committed, we can then do what this PR does
@HoustonPutman A change like that risks Haythem having to start over an additional time. I am encouraging him to get this done quickly so avoid it getting stale as what happened last time (which was depressing). If we want to have solrj/core then I propose that happen in this issue at the end, or in a follow-on PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1214583355
@sigram There is an abstraction `SolrCloudManager` and some related classes (DistributedQueue, DistribStateManager, NodeStateProvider) that you introduced once upon a time that don't seem to have anything to do with SolrJ yet you put them there. I believe these could go to Solr Core. Most need to _at least_ go to Solrj-zookeeper because of DistributedQueue's reference of ZK classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952727818
##########
solr/solrj/build.gradle:
##########
@@ -50,20 +41,16 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
- exclude group: "org.apache.yetus", module: "audience-annotations"
- exclude group: "log4j", module: "log4j"
- exclude group: "org.slf4j", module: "slf4j-log4j12"
- })
- implementation ('org.apache.zookeeper:zookeeper-jute') {
- exclude group: 'org.apache.yetus', module: 'audience-annotations'
- }
-
testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
testImplementation project(':solr:solrj')
testRuntimeOnly project(':solr:modules:sql')
+ // ideally ZK centric tests move to solrj-zookeeper but sometimes we depend on ZK here anyway
+ testImplementation project(':solr:solrj-zookeeper')
+ testImplementation 'org.apache.zookeeper:zookeeper'
+ permitTestUnusedDeclared 'org.apache.zookeeper:zookeeper'
Review Comment:
Ahhh good to know, thanks for the context!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952553934
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Solrj-ZooKeeper - SolrJ requiring ZooKeeper'
+
+dependencies {
+ // Spotbugs Annotations are only needed for old findbugs
+ // annotation usage like in Zookeeper during compilation time.
+ // It is not included in the release so exclude from checks.
+ compileOnly 'com.github.spotbugs:spotbugs-annotations'
+ permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
+ // Exclude these from jar validation and license checks.
+ configurations.jarValidation {
+ exclude group: "com.github.spotbugs", module: "spotbugs-annotations"
+ }
+
+ implementation project(':solr:solrj')
+
+ // declare dependencies we use even though already declared by solrj-core
+ implementation 'org.slf4j:slf4j-api'
+ implementation 'org.apache.httpcomponents:httpclient'
+ implementation 'org.apache.httpcomponents:httpcore'
+
+ implementation('org.apache.zookeeper:zookeeper', {
+ exclude group: "org.apache.yetus", module: "audience-annotations"
+ exclude group: "log4j", module: "log4j"
+ exclude group: "org.slf4j", module: "slf4j-log4j12"
+ })
+ implementation ('org.apache.zookeeper:zookeeper-jute') {
+ exclude group: 'org.apache.yetus', module: 'audience-annotations'
+ }
+
+ testImplementation project(':solr:test-framework')
+ testImplementation project(':solr:core')
+
+ testImplementation 'junit:junit'
+ testImplementation 'commons-io:commons-io'
+ testImplementation 'com.google.guava:guava'
+
+ permitTestUsedUndeclared project(':solr:solrj-zookeeper') // duh!
Review Comment:
I see that you just did this to get the build to pass, but it's definitely odd. Not a complaint, just confusion.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkDistribStateManager.java:
##########
@@ -44,6 +48,21 @@ public ZkDistribStateManager(SolrZkClient zkClient) {
this.zkClient = zkClient;
}
+ @SuppressWarnings({"unchecked"})
+ @Override
+ public Map<String, Object> getJson(String path)
+ throws InterruptedException, IOException, KeeperException {
+ VersionedData data;
+ try {
+ data = getData(path);
+ } catch (KeeperException.NoNodeException | NoSuchElementException e) {
+ return Collections.emptyMap();
+ }
+ if (data == null || data.getData() == null || data.getData().length == 0)
Review Comment:
Can we add `{}` here?
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Solrj-ZooKeeper - SolrJ requiring ZooKeeper'
+
+dependencies {
+ // Spotbugs Annotations are only needed for old findbugs
+ // annotation usage like in Zookeeper during compilation time.
+ // It is not included in the release so exclude from checks.
+ compileOnly 'com.github.spotbugs:spotbugs-annotations'
+ permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
+ // Exclude these from jar validation and license checks.
+ configurations.jarValidation {
+ exclude group: "com.github.spotbugs", module: "spotbugs-annotations"
+ }
+
+ implementation project(':solr:solrj')
+
+ // declare dependencies we use even though already declared by solrj-core
+ implementation 'org.slf4j:slf4j-api'
+ implementation 'org.apache.httpcomponents:httpclient'
+ implementation 'org.apache.httpcomponents:httpcore'
+
+ implementation('org.apache.zookeeper:zookeeper', {
+ exclude group: "org.apache.yetus", module: "audience-annotations"
+ exclude group: "log4j", module: "log4j"
+ exclude group: "org.slf4j", module: "slf4j-log4j12"
+ })
+ implementation ('org.apache.zookeeper:zookeeper-jute') {
+ exclude group: 'org.apache.yetus', module: 'audience-annotations'
+ }
+
+ testImplementation project(':solr:test-framework')
+ testImplementation project(':solr:core')
+
+ testImplementation 'junit:junit'
+ testImplementation 'commons-io:commons-io'
+ testImplementation 'com.google.guava:guava'
+
+ permitTestUsedUndeclared project(':solr:solrj-zookeeper') // duh!
Review Comment:
hmm not sure I understand this, since we are actively using solrj-zookeeper classes in the tests... Did I miss some discussion already?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -67,6 +72,50 @@ public ZkClientClusterStateProvider(String zkHost) {
this.zkHost = zkHost;
}
+ /**
+ * Create a ClusterState from Json. This method supports legacy configName location
+ *
+ * @param bytes a byte array of a Json representation of a mapping from collection name to the
+ * Json representation of a {@link DocCollection} as written by {@link
+ * ClusterState#write(JSONWriter)}. It can represent one or more collections.
+ * @param liveNodes list of live nodes
+ * @param coll collection name
+ * @param zkClient ZK client
+ * @return the ClusterState
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonSupportingLegacyConfigName(
+ int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
+ if (bytes == null || bytes.length == 0) {
+ return new ClusterState(liveNodes, Collections.emptyMap());
+ }
+ Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(bytes);
Review Comment:
@dsmiley @heythm is this an issue or can we ignore it?
##########
solr/solrj/build.gradle:
##########
@@ -50,20 +41,16 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
- exclude group: "org.apache.yetus", module: "audience-annotations"
- exclude group: "log4j", module: "log4j"
- exclude group: "org.slf4j", module: "slf4j-log4j12"
- })
- implementation ('org.apache.zookeeper:zookeeper-jute') {
- exclude group: 'org.apache.yetus', module: 'audience-annotations'
- }
-
testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
testImplementation project(':solr:solrj')
testRuntimeOnly project(':solr:modules:sql')
+ // ideally ZK centric tests move to solrj-zookeeper but sometimes we depend on ZK here anyway
+ testImplementation project(':solr:solrj-zookeeper')
+ testImplementation 'org.apache.zookeeper:zookeeper'
+ permitTestUnusedDeclared 'org.apache.zookeeper:zookeeper'
Review Comment:
We should be inheriting `zookeeper` from `solrj-zookeeper`. Since it is an `implementation` dependency there, the only use I can see for it here is if we are using APIs, but it looks like it's unused? If so why can't we just use the transitive dependency from `solrj-zookeeper`?
##########
solr/core/src/java/org/apache/solr/logging/MDCLoggingContext.java:
##########
@@ -108,6 +108,16 @@ private static void setNodeName(String node) {
}
}
+ public static String getNodeName() {
+ String s = MDC.get(NODE_NAME_PROP);
+ if (s == null) return null;
+ if (s.startsWith("n:")) {
Review Comment:
why does this start with "n:"? In the method above, it just set's value as the node name.
##########
solr/core/src/java/org/apache/solr/logging/MDCLoggingContext.java:
##########
@@ -108,6 +108,16 @@ private static void setNodeName(String node) {
}
}
+ public static String getNodeName() {
+ String s = MDC.get(NODE_NAME_PROP);
+ if (s == null) return null;
+ if (s.startsWith("n:")) {
Review Comment:
Just saw that this was copied and pasted from elsewhere, so I guess ignore...
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -67,7 +67,51 @@ public ZkClientClusterStateProvider(String zkHost) {
this.zkHost = zkHost;
}
- @Override
+ /**
+ * Create a ClusterState from Json. This method supports legacy configName location
+ *
+ * @param bytes a byte array of a Json representation of a mapping from collection name to the
+ * Json representation of a {@link DocCollection} as written by {@link ClusterState#write(JSONWriter)}. It
+ * can represent one or more collections.
+ * @param liveNodes list of live nodes
+ * @param coll collection name
+ * @param zkClient ZK client
+ * @return the ClusterState
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonSupportingLegacyConfigName(
Review Comment:
That's @janhoy 's PR, I don't have an opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r947196292
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -68,7 +69,21 @@ protected CloudHttp2SolrClient(Builder builder) {
"Both zkHost(s) & solrUrl(s) have been specified. Only specify one.");
}
if (builder.zkHosts != null) {
- this.stateProvider = new ZkClientClusterStateProvider(builder.zkHosts, builder.zkChroot);
+ try {
Review Comment:
Worth some docs on this? Why do we need to use the Class.forName, is this potentially pluggable somehow? Makes me think of JDBC drivers!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1185686001
Thanks for getting this started again @haythemkh! As a first-pass I suggest that we put the current solrj functionality into `solrj/core`. Once that is committed, we can then do what this PR does, but split it out into `solrj/zookeeper` instead of `solrj-zookeeper`. I do not think we want to have all of the `solrj` libraries as folders under `solr/` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1199648743
oops, was trying to be helpful resolving the merge conflict but forgot to compile locally before pushing, sorry. will try to fix, or else commit reversion logically should also be possible? sorry for the noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952746247
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
Does this zkStateReader need to be closed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952713889
##########
solr/solrj/build.gradle:
##########
@@ -50,20 +41,16 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
- exclude group: "org.apache.yetus", module: "audience-annotations"
- exclude group: "log4j", module: "log4j"
- exclude group: "org.slf4j", module: "slf4j-log4j12"
- })
- implementation ('org.apache.zookeeper:zookeeper-jute') {
- exclude group: 'org.apache.yetus', module: 'audience-annotations'
- }
-
testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
testImplementation project(':solr:solrj')
testRuntimeOnly project(':solr:modules:sql')
+ // ideally ZK centric tests move to solrj-zookeeper but sometimes we depend on ZK here anyway
+ testImplementation project(':solr:solrj-zookeeper')
+ testImplementation 'org.apache.zookeeper:zookeeper'
+ permitTestUnusedDeclared 'org.apache.zookeeper:zookeeper'
Review Comment:
@risdenk recently added a plugin to our Gradle build that demands we declare all dependencies of a compile time nature, even transitives. This obviously adds additional lines across our build to maintain. I wish it was smarter, for example, don't do this for Gradle's rather unique "api". And it has bugs where we have to add "permitBlahBlahBlah" where it shouldn't be necessary ideally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952809910
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
No; it was simply extracted from the CloudSolrClient's `ZkClientClusterStateProvider`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1219456482
:shipit: ???
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1214405668
> * In our javadocs, stop **linking** out to solrj or solrj-zookeeper because they cannot be differentiated by the standard javadoc tool.
This seems acceptable for 9.x, and in main branch we could change package names and re-introduce some links if needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r933893619
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -125,6 +127,14 @@ public JavaBinCodec() {
writableDocFields = null;
}
+ public static InputStream toJavabin(Object o) throws IOException {
Review Comment:
While we're moving this, I suggest calling this method `serializeToInputStream`. The word "serialize" is perfect, and conversion methods ideally indicate the return type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r946717649
##########
gradle/documentation/render-javadoc.gradle:
##########
@@ -141,13 +141,26 @@ configure(project(":solr:solrj")) {
}
}
+configure(project(":solr:solrj-zookeeper")) {
+ project.tasks.withType(RenderJavadocTask) {
+ // TODO: clean up split packages
Review Comment:
Could mention [SOLR-15604](https://issues.apache.org/jira/browse/SOLR-15604) here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1219456291
Docs look good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952714862
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Solrj-ZooKeeper - SolrJ requiring ZooKeeper'
+
+dependencies {
+ // Spotbugs Annotations are only needed for old findbugs
+ // annotation usage like in Zookeeper during compilation time.
+ // It is not included in the release so exclude from checks.
+ compileOnly 'com.github.spotbugs:spotbugs-annotations'
+ permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
+ // Exclude these from jar validation and license checks.
+ configurations.jarValidation {
+ exclude group: "com.github.spotbugs", module: "spotbugs-annotations"
+ }
+
+ implementation project(':solr:solrj')
+
+ // declare dependencies we use even though already declared by solrj-core
+ implementation 'org.slf4j:slf4j-api'
+ implementation 'org.apache.httpcomponents:httpclient'
+ implementation 'org.apache.httpcomponents:httpcore'
+
+ implementation('org.apache.zookeeper:zookeeper', {
+ exclude group: "org.apache.yetus", module: "audience-annotations"
+ exclude group: "log4j", module: "log4j"
+ exclude group: "org.slf4j", module: "slf4j-log4j12"
+ })
+ implementation ('org.apache.zookeeper:zookeeper-jute') {
+ exclude group: 'org.apache.yetus', module: 'audience-annotations'
+ }
+
+ testImplementation project(':solr:test-framework')
+ testImplementation project(':solr:core')
+
+ testImplementation 'junit:junit'
+ testImplementation 'commons-io:commons-io'
+ testImplementation 'com.google.guava:guava'
+
+ permitTestUsedUndeclared project(':solr:solrj-zookeeper') // duh!
Review Comment:
Very odd indeed! See the previous comment I added a minute ago. CC @risdenk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952720997
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -67,6 +72,50 @@ public ZkClientClusterStateProvider(String zkHost) {
this.zkHost = zkHost;
}
+ /**
+ * Create a ClusterState from Json. This method supports legacy configName location
+ *
+ * @param bytes a byte array of a Json representation of a mapping from collection name to the
+ * Json representation of a {@link DocCollection} as written by {@link
+ * ClusterState#write(JSONWriter)}. It can represent one or more collections.
+ * @param liveNodes list of live nodes
+ * @param coll collection name
+ * @param zkClient ZK client
+ * @return the ClusterState
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonSupportingLegacyConfigName(
+ int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
+ if (bytes == null || bytes.length == 0) {
+ return new ClusterState(liveNodes, Collections.emptyMap());
+ }
+ Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(bytes);
Review Comment:
We can't have Utils.getJson depending on ZK. There was one such method but it was only used in one place so we basically inlined the implementation. I don't think @heythm wrote this code; it just moved.
I am curious about what Lift's analysis sees; I'll look closer.
##########
solr/core/src/java/org/apache/solr/logging/MDCLoggingContext.java:
##########
@@ -108,6 +108,16 @@ private static void setNodeName(String node) {
}
}
+ public static String getNodeName() {
+ String s = MDC.get(NODE_NAME_PROP);
+ if (s == null) return null;
+ if (s.startsWith("n:")) {
Review Comment:
Yes; just moved the method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r952738945
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Solrj-ZooKeeper - SolrJ requiring ZooKeeper'
+
+dependencies {
+ // Spotbugs Annotations are only needed for old findbugs
+ // annotation usage like in Zookeeper during compilation time.
+ // It is not included in the release so exclude from checks.
+ compileOnly 'com.github.spotbugs:spotbugs-annotations'
+ permitUnusedDeclared 'com.github.spotbugs:spotbugs-annotations'
+ // Exclude these from jar validation and license checks.
+ configurations.jarValidation {
+ exclude group: "com.github.spotbugs", module: "spotbugs-annotations"
+ }
+
+ implementation project(':solr:solrj')
+
+ // declare dependencies we use even though already declared by solrj-core
+ implementation 'org.slf4j:slf4j-api'
+ implementation 'org.apache.httpcomponents:httpclient'
+ implementation 'org.apache.httpcomponents:httpcore'
+
+ implementation('org.apache.zookeeper:zookeeper', {
+ exclude group: "org.apache.yetus", module: "audience-annotations"
+ exclude group: "log4j", module: "log4j"
+ exclude group: "org.slf4j", module: "slf4j-log4j12"
+ })
+ implementation ('org.apache.zookeeper:zookeeper-jute') {
+ exclude group: 'org.apache.yetus', module: 'audience-annotations'
+ }
+
+ testImplementation project(':solr:test-framework')
+ testImplementation project(':solr:core')
+
+ testImplementation 'junit:junit'
+ testImplementation 'commons-io:commons-io'
+ testImplementation 'com.google.guava:guava'
+
+ permitTestUsedUndeclared project(':solr:solrj-zookeeper') // duh!
Review Comment:
I don't quite understand this one. We only do this for solr/webapp and solr:core. We don't have any existing ones that use `permitTestUsedUndeclared` in the same module where its defined...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1206936702
PerReplicaStates is referenced in ClusterState, DocCollection, and Replica. Based on my very limited understanding of PRS, it's a part of the state of a collection no matter how it's retrieved (via ZK or via SolrClient based ClusterStateProvider). Am I right @noblepaul @chatman ?
I do like your suggestion of those 3 "Props" classes for constants. How do we get there from here? Directly, even if it changes import statement in lots more classes (I would assume)? This is the easiest path. Or do we leave that for another JIRA issue (soon) and for now just remove the nocommits about this to make precommit happy? Also relatively easy but leaves some WIP. Or go part way here & now -- add the new classes & constants but only use them in solrj-zookeeper right now and then migrate the rest in another issue? More work.
I would like to point out that this new separation did move some tests to solrj-zookeeper but it's only four -- that's it. Many solrj tests indirectly use ZK and I suppose that's okay. There is a compile time dependency on ZK from solrj tests that is unfortunate but couldn't be avoided in a few cases (without investing additional work):
* StreamingTest.streamLocalTests calls `zkStateReader.forceUpdateCollection`
* TestCloudSolrClientConnections calls cluster.getZkClient()
* org.apache.solr.client.solrj.impl.HttpClusterStateSSLTest#testHttpClusterStateWithSSL calls cluster.getZkClient()
Perhaps these might even move but on the other hand, it's not some big deal to add an explicit dependency on solrj tests to ZK when ZK is definitely already there at runtime via the solrj-zookeeper dependency.
There is a weird dependency check problem that I can't figure out:
```
Execution failed for task ':solr:solrj-zookeeper:analyzeTestClassesDependencies'.
> Dependency analysis found issues.
usedUndeclaredArtifacts
- org.apache.solr:solrj-zookeeper:10.0.0-SNAPSHOT@
```
So solrj-zookeeper's tests depend on solrj-zookeeper. Well of course it does ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1222663050
I think it's done and I've addressed comments. I would love to see this merged soon so as to unblock other efforts that touch lots of files. If nobody has further review comments, I'll merge it Thursday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r954043634
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
We shouldn't do that. `ZkStateReader.from` is a glorified "get" but keeps ZooKeeper dependencies off of CloudSolrClient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r953987426
##########
solr/core/src/test/org/apache/solr/cloud/ReplaceNodeTest.java:
##########
@@ -115,9 +115,9 @@ public void test() throws Exception {
log.info("excluded_node : {} ", emptyNode);
createReplaceNodeRequest(nodeToBeDecommissioned, emptyNode, null)
.processAndWait("000", cloudClient, 15);
+ ZkStateReader zkStateReader = ZkStateReader.from(cloudClient);
Review Comment:
Does it hurt to close the ZkStateReader? Wrap in try-with-resources?
```
try (ZkStateReader zkStateReader = ZkStateReader.from(cloudClient)) {
...
}
```
I'm not that worried about the name, but want to make sure if things are closable they are closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] haythemkh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
haythemkh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r930595757
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -293,7 +293,7 @@ public static Slice[] getSlices(
// check for alias or collection
- List<String> allCollections = new ArrayList<>();
+ List<String> allCollections = new ArrayLigist<>();
Review Comment:
Yes. This is already fixed in the PR :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] haythemkh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
haythemkh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r930594836
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -282,50 +277,6 @@ public List<TupleStream> children() {
return solrStreams;
}
- public static Slice[] getSlices(
Review Comment:
I moved this to CloudSolrStream
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1206637357
> [PerReplicaStates](https://github.com/apache/solr/blob/521900d77ac938ba98a237fc1ca77700b168714d/solr/solrj/src/java/org/apache/solr/common/cloud/PerReplicaStates.java#L182)
Is there any reason for PRS not to be in the zk module? I don't think it would be used if we aren't communicating with ZK.
> Copy of ZkStateReader constants to solrj classes
Would it make more sense to have one `ClusterStateProps` class to hold these? We could also split it into `ClusterProps`, `CollectionProps`, etc, but that might be overkill.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r947195525
##########
solr/solrj/src/java/org/apache/solr/common/cloud/CollectionStatePredicate.java:
##########
@@ -18,14 +18,12 @@
package org.apache.solr.common.cloud;
import java.util.Set;
-import java.util.concurrent.TimeUnit;
-import java.util.function.Predicate;
/**
* Interface to determine if a set of liveNodes and a collection's state matches some expecatations.
*
- * @see ZkStateReader#waitForState(String, long, TimeUnit, CollectionStatePredicate)
- * @see ZkStateReader#waitForState(String, long, TimeUnit, Predicate)
+ * <p>{@code ZkStateReader#waitForState(String, long, TimeUnit, CollectionStatePredicate)} {@code
Review Comment:
I see an open <p> but not a close one? Should it be just two bullets instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org