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/12/30 22:21:26 UTC
[GitHub] [solr] justinrsweeney opened a new pull request, #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
justinrsweeney opened a new pull request, #1258:
URL: https://github.com/apache/solr/pull/1258
https://issues.apache.org/jira/browse/SOLR-XXXXX
<!--
_(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
Please provide a short description of the changes you're making with this pull request.
# 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] noblepaul commented on a diff in pull request #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
Posted by GitBox <gi...@apache.org>.
noblepaul commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1059804588
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -105,7 +104,6 @@ public class ZkStateReader implements SolrCloseable {
public static final String MAX_WAIT_SECONDS_PROP = "maxWaitSeconds";
public static final String STATE_TIMESTAMP_PROP = "stateTimestamp";
public static final String COLLECTIONS_ZKNODE = "/collections";
- public static final String LIVE_NODES_ZKNODE = "/live_nodes";
Review Comment:
I know, but it's in SolrJ. So, it could be used by others . It doesn't hurt us to just keep it around as `@Deprecated`
--
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 #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1059710790
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkLiveNodes.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptySortedSet;
+
+import java.lang.invoke.MethodHandles;
+import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.common.SolrException;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZkLiveNodes implements Watcher {
+ public static final String LIVE_NODES_ZKNODE = "/live_nodes";
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private volatile SortedSet<String> liveNodes = emptySortedSet();
+ private final Set<LiveNodesListener> liveNodesListeners = ConcurrentHashMap.newKeySet();
+ private final SolrZkClient zkClient;
+
+ public ZkLiveNodes(SolrZkClient zkClient) {
+ this.zkClient = zkClient;
+ }
+
+ public SortedSet<String> getLiveNodes() {
+ return liveNodes;
+ }
+
+ protected void setLiveNodes(SortedSet<String> liveNodes) {
+ this.liveNodes = liveNodes;
+ }
+
+ public Set<LiveNodesListener> getLiveNodesListeners() {
+ return liveNodesListeners;
+ }
+
+ public void addLiveNodesListener(LiveNodesListener liveNodesListener) {
+ this.liveNodesListeners.add(liveNodesListener);
+ }
+
+ public void removeLiveNodesListener(LiveNodesListener liveNodesListener) {
+ this.liveNodesListeners.remove(liveNodesListener);
+ }
+
+ /* Refresh live nodes */
+ public synchronized void refreshLiveNodes() throws KeeperException, InterruptedException {
Review Comment:
I think the methods on this class don't need "LiveNodes" in them.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -679,7 +679,13 @@ private void constructState(Set<String> changedCollections) {
result.putIfAbsent(entry.getKey(), entry.getValue());
}
- this.clusterState = new ClusterState(result, liveNodes);
+ this.clusterState = new ClusterState(result, zkLiveNodes.getLiveNodes());
+ // Listen for future live nodes changes to update ClusterState
+ zkLiveNodes.addLiveNodesListener(
+ (o, n) -> {
+ clusterState.setLiveNodes(n);
+ return false;
+ });
Review Comment:
is this new?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -105,7 +104,6 @@ public class ZkStateReader implements SolrCloseable {
public static final String MAX_WAIT_SECONDS_PROP = "maxWaitSeconds";
public static final String STATE_TIMESTAMP_PROP = "stateTimestamp";
public static final String COLLECTIONS_ZKNODE = "/collections";
- public static final String LIVE_NODES_ZKNODE = "/live_nodes";
Review Comment:
Shrug; it's also rather internal to SolrCloud internals.
--
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] noblepaul commented on a diff in pull request #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
Posted by GitBox <gi...@apache.org>.
noblepaul commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1059698710
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -105,7 +104,6 @@ public class ZkStateReader implements SolrCloseable {
public static final String MAX_WAIT_SECONDS_PROP = "maxWaitSeconds";
public static final String STATE_TIMESTAMP_PROP = "stateTimestamp";
public static final String COLLECTIONS_ZKNODE = "/collections";
- public static final String LIVE_NODES_ZKNODE = "/live_nodes";
Review Comment:
It's backward incompatible to remove this public static variable. May be we can deprecate this and remove later
--
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] justinrsweeney commented on a diff in pull request #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
Posted by GitBox <gi...@apache.org>.
justinrsweeney commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1060209357
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -679,7 +679,13 @@ private void constructState(Set<String> changedCollections) {
result.putIfAbsent(entry.getKey(), entry.getValue());
}
- this.clusterState = new ClusterState(result, liveNodes);
+ this.clusterState = new ClusterState(result, zkLiveNodes.getLiveNodes());
+ // Listen for future live nodes changes to update ClusterState
+ zkLiveNodes.addLiveNodesListener(
+ (o, n) -> {
+ clusterState.setLiveNodes(n);
+ return false;
+ });
Review Comment:
This is new, but none of this is really tested yet.
The idea I'm thinking here is to create the ClusterState once and then have classes for each data type in ZK handling updates and updating necessary places via listeners like here: https://github.com/apache/solr/pull/1258/files/144d380843439672b2b7e76dae6fc6c94f839924#diff-4710a20b3d893bec0637610d3cce7dc383ca328c68a6fb0d4c7266e2041b9d0bR90, instead of having a bunch of methods in ZkStateReader that update ClusterState for all sort of data types.
--
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] noblepaul commented on a diff in pull request #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
Posted by GitBox <gi...@apache.org>.
noblepaul commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1059804623
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkLiveNodes.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptySortedSet;
+
+import java.lang.invoke.MethodHandles;
+import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.common.SolrException;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZkLiveNodes implements Watcher {
+ public static final String LIVE_NODES_ZKNODE = "/live_nodes";
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private volatile SortedSet<String> liveNodes = emptySortedSet();
+ private final Set<LiveNodesListener> liveNodesListeners = ConcurrentHashMap.newKeySet();
+ private final SolrZkClient zkClient;
+
+ public ZkLiveNodes(SolrZkClient zkClient) {
+ this.zkClient = zkClient;
+ }
+
+ public SortedSet<String> getLiveNodes() {
+ return liveNodes;
+ }
+
+ protected void setLiveNodes(SortedSet<String> liveNodes) {
+ this.liveNodes = liveNodes;
+ }
+
+ public Set<LiveNodesListener> getLiveNodesListeners() {
+ return liveNodesListeners;
+ }
+
+ public void addLiveNodesListener(LiveNodesListener liveNodesListener) {
+ this.liveNodesListeners.add(liveNodesListener);
+ }
+
+ public void removeLiveNodesListener(LiveNodesListener liveNodesListener) {
+ this.liveNodesListeners.remove(liveNodesListener);
+ }
+
+ /* Refresh live nodes */
+ public synchronized void refreshLiveNodes() throws KeeperException, InterruptedException {
Review Comment:
true
--
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] noblepaul commented on a diff in pull request #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class
Posted by GitBox <gi...@apache.org>.
noblepaul commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1059804623
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkLiveNodes.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.
+ */
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptySortedSet;
+
+import java.lang.invoke.MethodHandles;
+import java.util.List;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.solr.common.SolrException;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZkLiveNodes implements Watcher {
+ public static final String LIVE_NODES_ZKNODE = "/live_nodes";
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private volatile SortedSet<String> liveNodes = emptySortedSet();
+ private final Set<LiveNodesListener> liveNodesListeners = ConcurrentHashMap.newKeySet();
+ private final SolrZkClient zkClient;
+
+ public ZkLiveNodes(SolrZkClient zkClient) {
+ this.zkClient = zkClient;
+ }
+
+ public SortedSet<String> getLiveNodes() {
+ return liveNodes;
+ }
+
+ protected void setLiveNodes(SortedSet<String> liveNodes) {
+ this.liveNodes = liveNodes;
+ }
+
+ public Set<LiveNodesListener> getLiveNodesListeners() {
+ return liveNodesListeners;
+ }
+
+ public void addLiveNodesListener(LiveNodesListener liveNodesListener) {
+ this.liveNodesListeners.add(liveNodesListener);
+ }
+
+ public void removeLiveNodesListener(LiveNodesListener liveNodesListener) {
+ this.liveNodesListeners.remove(liveNodesListener);
+ }
+
+ /* Refresh live nodes */
+ public synchronized void refreshLiveNodes() throws KeeperException, InterruptedException {
Review Comment:
true. just `refresh()` is enough
--
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