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/02 18:18:49 UTC
[GitHub] [solr] nginthfs opened a new pull request, #1208: SOLR-16571: Add java system property to disable config watch
nginthfs opened a new pull request, #1208:
URL: https://github.com/apache/solr/pull/1208
# Description
Solr creates a ZooKeeper watch per-core. On nodes with a lot of cores, this results in a lot of ZK watches, and thus increased resource consumption. ZK config watches aren't strictly necessary, so we should be able to disable them if we want to.
# Solution
To remedy this, I've added a Java system property to disable the ZK config watch per core. The default behavior is still to create a ZK config watch per core.
# Tests
I've added a test confirming that the ZK watch is not created when the system property is set to true.
# 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 #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
noblepaul commented on code in PR #1208:
URL: https://github.com/apache/solr/pull/1208#discussion_r1052998981
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -2736,6 +2736,12 @@ private Set<Runnable> getConfDirListeners(final String confDir) {
return confDirListeners;
}
+ public boolean hasConfDirectoryListeners(final String confDir) {
Review Comment:
so, this method was added just for the JUnit? do we not have any other way to know if there is a watch from Zookeeper client?
--
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] nginthfs commented on a diff in pull request #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
nginthfs commented on code in PR #1208:
URL: https://github.com/apache/solr/pull/1208#discussion_r1041166363
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -188,6 +188,7 @@
public class SolrCore implements SolrInfoBean, Closeable {
public static final String version = "1.0";
+ public static final String DISABLE_ZK_CONFIG_WATCH = "disable.zk.config.watch";
Review Comment:
Would `configSet.core.watch.disable` be okay, or should I change the logic to check for the Boolean and default to false?
--
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 #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1208:
URL: https://github.com/apache/solr/pull/1208#discussion_r1038444137
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -2736,6 +2736,10 @@ private Set<Runnable> getConfDirListeners(final String confDir) {
return confDirListeners;
}
+ public boolean hasConfDirectoryListeners(final String confDir) {
+ return confDirectoryListeners.containsKey(confDir) && !confDirectoryListeners.isEmpty();
Review Comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `ZkController.hasConfDirectoryListeners(...)` reads without synchronization from container `this.confDirectoryListeners` via call to `Map.isEmpty()`. Potentially races with write in method `ZkController.registerConfListenerForCore(...)`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=357510788&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=357510788&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=357510788&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=357510788&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=357510788&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] dsmiley commented on a diff in pull request #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1208:
URL: https://github.com/apache/solr/pull/1208#discussion_r1041324608
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -188,6 +188,7 @@
public class SolrCore implements SolrInfoBean, Closeable {
public static final String version = "1.0";
+ public static final String DISABLE_ZK_CONFIG_WATCH = "disable.zk.config.watch";
Review Comment:
Yeah, you're right. The boolean should represent the change from its default state.
--
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 #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1208:
URL: https://github.com/apache/solr/pull/1208#issuecomment-1345468456
Yeah so just move the test to the existing TestConfigReload and let's be done with it. If you want to increase the scope to mark these things deprecated, that's be nice!
CC @noblepaul
--
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 #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1208:
URL: https://github.com/apache/solr/pull/1208#issuecomment-1343837114
Let's see where my comment goes on the parent JIRA. Note that this test fails no matter your change to no-op it :-). It's because you didn't actually create any collections. As we take this further, I found a good home for this test -- org.apache.solr.handler.TestConfigReload. Yes it has the wrong base class; maybe you were inspired from this one originally.
--
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 pull request #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
noblepaul commented on PR #1208:
URL: https://github.com/apache/solr/pull/1208#issuecomment-1358960228
`If you want to increase the scope to mark these things deprecated, that's be nice!`
I'll do that .
`so just move the test to the existing TestConfigReload `
Yes, please
--
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 #1208: SOLR-16571: Add java system property to disable config watch
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1208:
URL: https://github.com/apache/solr/pull/1208#discussion_r1040469355
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -188,6 +188,7 @@
public class SolrCore implements SolrInfoBean, Closeable {
public static final String version = "1.0";
+ public static final String DISABLE_ZK_CONFIG_WATCH = "disable.zk.config.watch";
Review Comment:
I would prefer this be named something like `configSet.core.watch.enable`
##########
solr/core/src/test/org/apache/solr/core/SolrCoreConfigTest.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.core;
+
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.cloud.AbstractFullDistribZkTestBase;
+import org.junit.Test;
+
+public class SolrCoreConfigTest extends AbstractFullDistribZkTestBase {
Review Comment:
This test doesn't seem to need to subclass AbstractFullDistribZkTestBase, which is for distributed-search (sharing) testing. It appears you could use SolrCloudTestCase which is much simpler.
--
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