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/03/24 16:28:31 UTC
[GitHub] [solr] madrob opened a new pull request #759: SOLR-16114 Remove SolrZookeeper
madrob opened a new pull request #759:
URL: https://github.com/apache/solr/pull/759
ZooKeeper has sufficient APIs now for us to no longer need to maintain
our own wrapper client.
https://issues.apache.org/jira/browse/SOLR-16114
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834655528
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1886,7 +1886,7 @@ public void reload(String name, UUID coreId) {
if (docCollection != null) {
Replica replica = docCollection.getReplica(cd.getCloudDescriptor().getCoreNodeName());
- assert replica != null;
+ assert replica != null : cd.getCloudDescriptor().getCoreNodeName() + " had no replica";
Review comment:
This came in while I was trying to debug a failure that my changes introduces. I can take it back out, but figured more logging and error messages are likely better than less.
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834660338
##########
File path: solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
##########
@@ -300,6 +301,7 @@ public static void setupTestCases() {
System.setProperty("solr.v2RealPath", "true");
System.setProperty("zookeeper.forceSync", "no");
System.setProperty("jetty.testMode", "true");
+ System.setProperty("solr.zookeeper.connectionStrategy", TestConnectionStrategy.class.getName());
Review comment:
What about a comment on TestConnectionStrategy instead? See the changes for that in b386e144e2f
--
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 change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834511425
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java
##########
@@ -57,6 +57,7 @@
* All Solr ZooKeeper interactions should go through this class rather than ZooKeeper. This class
* handles synchronous connects and reconnections.
*/
+// The constructor overloads are a little awkward, it would be nice to move this to a builder
Review comment:
100%
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ChaosMonkey.java
##########
@@ -149,8 +152,7 @@ public void expireSession(final JettySolrRunner jetty) {
if (cores != null) {
monkeyLog("expire session for " + jetty.getLocalPort() + " !");
causeConnectionLoss(jetty);
- long sessionId = cores.getZkController().getZkClient().getSolrZooKeeper().getSessionId();
- zkServer.expire(sessionId);
+ cores.getZkController().getZkClient().getZooKeeper().getTestable().injectSessionExpiration();
Review comment:
do we not need to expire on the server as well?
--
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 change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834627698
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ChaosMonkey.java
##########
@@ -149,8 +152,7 @@ public void expireSession(final JettySolrRunner jetty) {
if (cores != null) {
monkeyLog("expire session for " + jetty.getLocalPort() + " !");
causeConnectionLoss(jetty);
- long sessionId = cores.getZkController().getZkClient().getSolrZooKeeper().getSessionId();
- zkServer.expire(sessionId);
+ cores.getZkController().getZkClient().getZooKeeper().getTestable().injectSessionExpiration();
Review comment:
Mike and I have determined that we do need to expire on the server still, so that ephemeral nodes don't take 30 seconds (the default `zkClientTimeout`) to be deleted.
--
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 a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834516072
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1886,7 +1886,7 @@ public void reload(String name, UUID coreId) {
if (docCollection != null) {
Replica replica = docCollection.getReplica(cd.getCloudDescriptor().getCoreNodeName());
- assert replica != null;
+ assert replica != null : cd.getCloudDescriptor().getCoreNodeName() + " had no replica";
Review comment:
minor: minor unrelated change?
##########
File path: solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
##########
@@ -300,6 +301,7 @@ public static void setupTestCases() {
System.setProperty("solr.v2RealPath", "true");
System.setProperty("zookeeper.forceSync", "no");
System.setProperty("jetty.testMode", "true");
+ System.setProperty("solr.zookeeper.connectionStrategy", TestConnectionStrategy.class.getName());
Review comment:
Could we have a comment here re: how or where it is used?
##########
File path: solr/core/src/test/org/apache/solr/cloud/DistributedQueueTest.java
##########
@@ -299,7 +299,7 @@ private void forceSessionExpire() throws InterruptedException, TimeoutException
Thread.sleep(50);
}
assertTrue(zkClient.isConnected());
- assertFalse(sessionId == zkClient.getSolrZooKeeper().getSessionId());
+ assertFalse(sessionId == zkClient.getZooKeeper().getSessionId());
Review comment:
pre-existing
```suggestion
assertNotEquals(sessionId, zkClient.getZooKeeper().getSessionId());
```
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java
##########
@@ -791,14 +791,13 @@ public JettySolrRunner getReplicaJetty(Replica replica) {
/** Make the zookeeper session on a particular jetty expire */
public void expireZkSession(JettySolrRunner jetty) {
+ ChaosMonkey.causeConnectionLoss(jetty);
+
Review comment:
Am curious about (not) keeping the connection loss/close within the `cores != null` conditional before the session expiry.
##########
File path: solr/licenses/zookeeper-3.7.0-tests.jar.sha1
##########
@@ -0,0 +1 @@
+093f2c34c33ee16f9ff3f9352c79eafd3cd9040a
Review comment:
dumb question: how to cross-check the sha1 value here? i couldn't find a `zookeeper-3.7.0-tests.jar` within http://archive.apache.org/dist/zookeeper/zookeeper-3.7.0/ say?
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r836500726
##########
File path: solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java
##########
@@ -531,32 +533,30 @@ public void run() {
};
Thread connLossThread =
- new Thread() {
- @Override
- public void run() {
-
- while (!stopStress) {
- try {
- Thread.sleep(50);
- int j;
- j = random().nextInt(threads.size());
+ new Thread(
+ () -> {
+ while (!stopStress) {
try {
- threads.get(j).es.zkClient.getSolrZooKeeper().closeCnxn();
- if (random().nextBoolean()) {
- long sessionId = zkClient.getSolrZooKeeper().getSessionId();
- server.expire(sessionId);
+ Thread.sleep(50);
+ int j;
+ j = random().nextInt(threads.size());
+ try {
+ ZooKeeper zk = threads.get(j).es.zkClient.getZooKeeper();
+ assertTrue(zk instanceof TestableZooKeeper);
+ ((TestableZooKeeper) zk).testableConnloss();
+ if (random().nextBoolean()) {
+ server.expire(zk.getSessionId());
Review comment:
I don't think they would, and I suspect this was a bug before. `zkClient` is the client for the text fixture itself, there should be no reason to be expiring the session on that one.
--
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 a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r835185774
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java
##########
@@ -113,7 +112,7 @@
class ZKServerMain {
private volatile ServerCnxnFactory cnxnFactory;
- private volatile ZooKeeperServer zooKeeperServer;
+ public volatile ZooKeeperServer zooKeeperServer;
Review comment:
minor: seems it could stay private?
```suggestion
private volatile ZooKeeperServer zooKeeperServer;
```
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkClientConnectionStrategy.java
##########
@@ -121,4 +122,35 @@ protected SolrZooKeeper createSolrZooKeeper(
return result;
}
+
+ // Override for testing
+ protected ZooKeeper newZooKeeperInstance(
+ final String serverAddress, final int zkClientTimeout, final Watcher watcher)
+ throws IOException {
+ return new ZooKeeper(serverAddress, zkClientTimeout, watcher);
+ }
+
+ /**
+ * Instantiate a new connection strategy for the given class name
+ *
+ * @param name the name of the strategy to use
+ * @return the strategy instance, or null if it could not be loaded
+ */
+ public static ZkClientConnectionStrategy forName(String name, ZkClientConnectionStrategy def) {
+ log.debug("Attempting to load zk connection strategy '{}'", name);
+ if (name == null) {
+ return def;
+ }
+
+ try {
+ // TODO should this use SolrResourceLoader?
Review comment:
Looks like `ZkController` via `CoreContainer` would have access to `SolrResourceLoader` but `SolrZkClient` wouldn't have since `SolrResourceLoader` isn't solrj.
##########
File path: solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java
##########
@@ -531,32 +533,30 @@ public void run() {
};
Thread connLossThread =
- new Thread() {
- @Override
- public void run() {
-
- while (!stopStress) {
- try {
- Thread.sleep(50);
- int j;
- j = random().nextInt(threads.size());
+ new Thread(
+ () -> {
+ while (!stopStress) {
try {
- threads.get(j).es.zkClient.getSolrZooKeeper().closeCnxn();
- if (random().nextBoolean()) {
- long sessionId = zkClient.getSolrZooKeeper().getSessionId();
- server.expire(sessionId);
+ Thread.sleep(50);
+ int j;
+ j = random().nextInt(threads.size());
+ try {
+ ZooKeeper zk = threads.get(j).es.zkClient.getZooKeeper();
+ assertTrue(zk instanceof TestableZooKeeper);
+ ((TestableZooKeeper) zk).testableConnloss();
+ if (random().nextBoolean()) {
+ server.expire(zk.getSessionId());
Review comment:
question: do we know/can we assume that `threads.get(j).es.zkClient` and `zkClient` use the same session id?
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834660108
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java
##########
@@ -791,14 +791,13 @@ public JettySolrRunner getReplicaJetty(Replica replica) {
/** Make the zookeeper session on a particular jetty expire */
public void expireZkSession(JettySolrRunner jetty) {
+ ChaosMonkey.causeConnectionLoss(jetty);
+
Review comment:
addressed in b386e144e2f
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ChaosMonkey.java
##########
@@ -149,8 +152,7 @@ public void expireSession(final JettySolrRunner jetty) {
if (cores != null) {
monkeyLog("expire session for " + jetty.getLocalPort() + " !");
causeConnectionLoss(jetty);
- long sessionId = cores.getZkController().getZkClient().getSolrZooKeeper().getSessionId();
- zkServer.expire(sessionId);
+ cores.getZkController().getZkClient().getZooKeeper().getTestable().injectSessionExpiration();
Review comment:
reverted in b386e144e2f
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834636827
##########
File path: solr/licenses/zookeeper-3.7.0-tests.jar.sha1
##########
@@ -0,0 +1 @@
+093f2c34c33ee16f9ff3f9352c79eafd3cd9040a
Review comment:
We can look at it on https://repository.apache.org/#nexus-search;gav~~zookeeper~3.7.0~~tests (scroll to the bottom pane)
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r834689783
##########
File path: solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
##########
@@ -300,6 +301,7 @@ public static void setupTestCases() {
System.setProperty("solr.v2RealPath", "true");
System.setProperty("zookeeper.forceSync", "no");
System.setProperty("jetty.testMode", "true");
+ System.setProperty("solr.zookeeper.connectionStrategy", TestConnectionStrategy.class.getName());
Review comment:
I realized that I was missing some changes necessary for this to make sense when I tried to split it out of a different branch that I had. Please take another look and see ZkClientConnectionStrategy
--
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 change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r836503361
##########
File path: solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java
##########
@@ -531,32 +533,30 @@ public void run() {
};
Thread connLossThread =
- new Thread() {
- @Override
- public void run() {
-
- while (!stopStress) {
- try {
- Thread.sleep(50);
- int j;
- j = random().nextInt(threads.size());
+ new Thread(
+ () -> {
+ while (!stopStress) {
try {
- threads.get(j).es.zkClient.getSolrZooKeeper().closeCnxn();
- if (random().nextBoolean()) {
- long sessionId = zkClient.getSolrZooKeeper().getSessionId();
- server.expire(sessionId);
+ Thread.sleep(50);
+ int j;
+ j = random().nextInt(threads.size());
+ try {
+ ZooKeeper zk = threads.get(j).es.zkClient.getZooKeeper();
+ assertTrue(zk instanceof TestableZooKeeper);
+ ((TestableZooKeeper) zk).testableConnloss();
+ if (random().nextBoolean()) {
+ server.expire(zk.getSessionId());
Review comment:
Ahhh sorry I forgot to mention this Mike, I had already fixed it on my curator branch when we were talking.
--
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] madrob commented on a change in pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #759:
URL: https://github.com/apache/solr/pull/759#discussion_r836479827
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java
##########
@@ -113,7 +112,7 @@
class ZKServerMain {
private volatile ServerCnxnFactory cnxnFactory;
- private volatile ZooKeeperServer zooKeeperServer;
+ public volatile ZooKeeperServer zooKeeperServer;
Review comment:
Yep, temporarily needed to make public while debugging other things, thanks.
--
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] madrob merged pull request #759: SOLR-16114 Remove SolrZookeeper
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #759:
URL: https://github.com/apache/solr/pull/759
--
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