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/08/23 12:30:41 UTC
[GitHub] [solr] HoustonPutman commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module
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