Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
aparnasuresh85 commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1687768687
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1213,10 +1193,6 @@ protected DocCollection getDocCollection(String
collection, Integer expectedVers
// no such collection exists
return null;
}
-if (!ref.isLazilyLoaded()) {
Review Comment:
> In summary, by bailing early here, we never even populated the cache. The
cache may not be important for a the ZK CSP because that has its own cache via
ZkStateReader holding and always updating the ClusterState. But this isn't true
of HTTP CSP and I don't think layering another cache there is the ideal
approach either. The "stateVer" protocol in place makes sense; embraces an
eventually-consistent mindset instead of pretending everything everywhere knows
all.
That is correct.
> By removing this check we'll get a _cached_ DocCollection, even for the ZK
CSP which has access to the latest state (albeit eventually consistent). This
is kind of sad, removing a benefit of the ZK CSP in that it's routing is
extremely closely up to date.
I believe ZK CSP always has its DocCollections cached in CSC since its works
with _Lazy_ refs. However, the `collectionStateCache` in CSC has a `TTL` of 60
seconds, I believe that is when this cache is updated to have the "latest"
state (get() on the lazy ref is invoked).
> Perhaps instead, put this back and modify HttpClusterStateProvider to
return a ClusterState.CollectionRef subclass that returns "true" for
isLazilyLoaded, and that which actually fetches the state in get() (thus lazily
not eagerly)?
I tried this approach initially, but found that this cannot be done without
adding another layer of cache in Http CSP.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
aparnasuresh85 commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1687768687
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1213,10 +1193,6 @@ protected DocCollection getDocCollection(String
collection, Integer expectedVers
// no such collection exists
return null;
}
-if (!ref.isLazilyLoaded()) {
Review Comment:
> In summary, by bailing early here, we never even populated the cache. The
cache may not be important for a the ZK CSP because that has its own cache via
ZkStateReader holding and always updating the ClusterState. But this isn't true
of HTTP CSP and I don't think layering another cache there is the ideal
approach either. The "stateVer" protocol in place makes sense; embraces an
eventually-consistent mindset instead of pretending everything everywhere knows
all.
That is correct.
> By removing this check we'll get a _cached_ DocCollection, even for the ZK
CSP which has access to the latest state (albeit eventually consistent). This
is kind of sad, removing a benefit of the ZK CSP in that it's routing is
extremely closely up to date.
I believe ZK CSP always has its DocCollections cached in CSC since its works
with _Lazy_ refs. However, the `collectionStateCache` in CSC has a `TTL` of 60
seconds, I believe that is when this cache is updated to have the "latest"
state (get() on the lazy ref is invoked).
> Perhaps instead, put this back and modify HttpClusterStateProvider to
return a ClusterState.CollectionRef subclass that returns "true" for
isLazilyLoaded, and that which actually fetches the state in get() (thus lazily
not eagerly)?
I tried this approach initially, but found that this cannot be done without
adding another layer of cache in Http CSP.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
aparnasuresh85 commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1687768687
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1213,10 +1193,6 @@ protected DocCollection getDocCollection(String
collection, Integer expectedVers
// no such collection exists
return null;
}
-if (!ref.isLazilyLoaded()) {
Review Comment:
> In summary, by bailing early here, we never even populated the cache. The
cache may not be important for a the ZK CSP because that has its own cache via
ZkStateReader holding and always updating the ClusterState. But this isn't true
of HTTP CSP and I don't think layering another cache there is the ideal
approach either. The "stateVer" protocol in place makes sense; embraces an
eventually-consistent mindset instead of pretending everything everywhere knows
all.
That is correct.
> By removing this check we'll get a _cached_ DocCollection, even for the ZK
CSP which has access to the latest state (albeit eventually consistent). This
is kind of sad, removing a benefit of the ZK CSP in that it's routing is
extremely closely up to date.
I believe ZK CSP always has its DocCollections cached in CSC since its works
with _Lazy_ refs. However, the `collectionStateCache` in CSC has a `TTL` of 60
seconds, I believe the cache is updated to have the "latest" state (get() on
the lazy ref is invoked) once the TTL expires.
> Perhaps instead, put this back and modify HttpClusterStateProvider to
return a ClusterState.CollectionRef subclass that returns "true" for
isLazilyLoaded, and that which actually fetches the state in get() (thus lazily
not eagerly)?
I tried this approach initially, but found that this cannot be done without
adding another layer of cache in Http CSP.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
aparnasuresh85 commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1687768687
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1213,10 +1193,6 @@ protected DocCollection getDocCollection(String
collection, Integer expectedVers
// no such collection exists
return null;
}
-if (!ref.isLazilyLoaded()) {
Review Comment:
> In summary, by bailing early here, we never even populated the cache. The
cache may not be important for a the ZK CSP because that has its own cache via
ZkStateReader holding and always updating the ClusterState. But this isn't true
of HTTP CSP and I don't think layering another cache there is the ideal
approach either. The "stateVer" protocol in place makes sense; embraces an
eventually-consistent mindset instead of pretending everything everywhere knows
all.
That is correct.
> By removing this check we'll get a _cached_ DocCollection, even for the ZK
CSP which has access to the latest state (albeit eventually consistent). This
is kind of sad, removing a benefit of the ZK CSP in that it's routing is
extremely closely up to date.
I believe ZK CSP always has its DocCollections cached in CSC since its works
with _Lazy_ refs. However, the `collectionStateCache` in CSC has a `TTL` of 60
seconds, I believe the cache is updated to have the "latest" state (get() on
the lazy ref is invoked) once the TTL expires.
> Perhaps instead, put this back and modify HttpClusterStateProvider to
return a ClusterState.CollectionRef subclass that returns "true" for
isLazilyLoaded, and that which actually fetches the state in get() (thus lazily
not eagerly)?
I tried this approach initially, but found that this cannot be done without
adding another layer of cache in Http CSP, and this cache would have its own
refresh rate to avoid caching stale entries.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Created] (SOLR-17376) Direct buffer memory leak in Solr Java Client
Alexander Veit created SOLR-17376:
-
Summary: Direct buffer memory leak in Solr Java Client
Key: SOLR-17376
URL: https://issues.apache.org/jira/browse/SOLR-17376
Project: Solr
Issue Type: Bug
Security Level: Public (Default Security Level. Issues are Public)
Components: clients - java, http2
Affects Versions: 9.6.1
Environment: Java version: 21.0.3+9-LTS
Windows Server 2019
Reporter: Alexander Veit
Attachments: image-2024-07-23-14-38-17-254.png
We use the Solr Java client in a log-running service to update documents in an
index. As it turns out the Solr client allocates direct buffers that accumulate
and never get released until JVM restart. After some hundreds of thousands
instances of java.nio.DirectByteBuffer haven been created the system gets stuck
and OutOfMemoryErrors occur inside the JVM. Unfortunately the JVM does not
terminate itself, probably because this is not an issue of Java heap memory.
Therefore the service could not be restarted automatically but requires user
interaction.
Although we have a rather complex scenario, we can almost certainly rule out
that the leak is due to missing close() calls on the SolrClient instances
within the JVM. SolrClients are generally reused during the lifetime of the JVM.
The Solr client was created like in the following code
{code:java}
SolrClient client = new Http2SolrClient.Builder("my-url").build();{code}
The Solr requests were made like in the following code
{code:java}
QueryRequest request = new QueryRequest(params);
request.process(client, "my-collection");{code}
A heap dump taken from an affected JVM showed the following instance counts:
{noformat}
org.apache.solr.client.solrj.impl.Http2SolrClient2
org.eclipse.jetty.io.LogarithmicArrayByteBufferPool 1
org.eclipse.jetty.io.MappedByteBufferPool2
java.nio.DirectByteBuffer 458485{noformat}
The DirectByteBuffers seem to be held alive by a thread-local map.
!image-2024-07-23-14-38-17-254.png!
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17376) Direct buffer memory leak in Solr Java Client
[
https://issues.apache.org/jira/browse/SOLR-17376?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Alexander Veit updated SOLR-17376:
--
Attachment: Bildschirmfoto vom 2024-07-18 13-23-06.png
Description:
We use the Solr Java client in a log-running service to update documents in an
index. As it turns out the Solr client allocates direct buffers that accumulate
and never get released until JVM restart. After some hundreds of thousands
instances of java.nio.DirectByteBuffer haven been created the system gets stuck
and OutOfMemoryErrors occur inside the JVM.
!Bildschirmfoto vom 2024-07-18 13-23-06.png!
Unfortunately the JVM does not terminate itself, probably because this is not
an issue of Java heap memory. Therefore the service could not be restarted
automatically but requires user interaction.
Although we have a rather complex scenario, we can almost certainly rule out
that the leak is due to missing close() calls on the SolrClient instances
within the JVM. SolrClients are generally reused during the lifetime of the JVM.
The Solr client was created like in the following code
{code:java}
SolrClient client = new Http2SolrClient.Builder("my-url").build();{code}
The Solr requests were made like in the following code
{code:java}
QueryRequest request = new QueryRequest(params);
request.process(client, "my-collection");{code}
A heap dump taken from an affected JVM showed the following instance counts:
{noformat}
org.apache.solr.client.solrj.impl.Http2SolrClient2
org.eclipse.jetty.io.LogarithmicArrayByteBufferPool 1
org.eclipse.jetty.io.MappedByteBufferPool2
java.nio.DirectByteBuffer 458485{noformat}
The DirectByteBuffers seem to be held alive by a thread-local map.
!image-2024-07-23-14-38-17-254.png!
was:
We use the Solr Java client in a log-running service to update documents in an
index. As it turns out the Solr client allocates direct buffers that accumulate
and never get released until JVM restart. After some hundreds of thousands
instances of java.nio.DirectByteBuffer haven been created the system gets stuck
and OutOfMemoryErrors occur inside the JVM. Unfortunately the JVM does not
terminate itself, probably because this is not an issue of Java heap memory.
Therefore the service could not be restarted automatically but requires user
interaction.
Although we have a rather complex scenario, we can almost certainly rule out
that the leak is due to missing close() calls on the SolrClient instances
within the JVM. SolrClients are generally reused during the lifetime of the JVM.
The Solr client was created like in the following code
{code:java}
SolrClient client = new Http2SolrClient.Builder("my-url").build();{code}
The Solr requests were made like in the following code
{code:java}
QueryRequest request = new QueryRequest(params);
request.process(client, "my-collection");{code}
A heap dump taken from an affected JVM showed the following instance counts:
{noformat}
org.apache.solr.client.solrj.impl.Http2SolrClient2
org.eclipse.jetty.io.LogarithmicArrayByteBufferPool 1
org.eclipse.jetty.io.MappedByteBufferPool2
java.nio.DirectByteBuffer 458485{noformat}
The DirectByteBuffers seem to be held alive by a thread-local map.
!image-2024-07-23-14-38-17-254.png!
> Direct buffer memory leak in Solr Java Client
> -
>
> Key: SOLR-17376
> URL: https://issues.apache.org/jira/browse/SOLR-17376
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Components: clients - java, http2
>Affects Versions: 9.6.1
> Environment: Java version: 21.0.3+9-LTS
> Windows Server 2019
>
>Reporter: Alexander Veit
>Priority: Major
> Labels: memory-bug
> Attachments: Bildschirmfoto vom 2024-07-18 13-23-06.png,
> image-2024-07-23-14-38-17-254.png
>
>
> We use the Solr Java client in a log-running service to update documents in
> an index. As it turns out the Solr client allocates direct buffers that
> accumulate and never get released until JVM restart. After some hundreds of
> thousands instances of java.nio.DirectByteBuffer haven been created the
> system gets stuck and OutOfMemoryErrors occur inside the JVM.
> !Bildschirmfoto vom 2024-07-18 13-23-06.png!
> Unfortunately the JVM does not terminate itself, probably because this is not
> an issue of Java heap memory. Therefore the service could not be restarted
> automatically but requires user interaction.
> Although we have a rather complex scenario, we can almost certainly rule out
> that the leak is due to missing close() calls on the SolrClient instances
> within the JVM. SolrClients are generally reused during the lifetime of the
> JVM.
[jira] [Commented] (SOLR-17376) Direct buffer memory leak in Solr Java Client
[
https://issues.apache.org/jira/browse/SOLR-17376?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868082#comment-17868082
]
Michael Gibney commented on SOLR-17376:
---
Depending on your usage pattern, [this
post|https://www.evanjones.ca/java-bytebuffer-leak.html] might be relevant? If
so, the sysprop {{-Djdk.nio.maxCachedBufferSize}} may help, and/or adjusting
workloads to not write large heap ByteBuffers all in one shot?
> Direct buffer memory leak in Solr Java Client
> -
>
> Key: SOLR-17376
> URL: https://issues.apache.org/jira/browse/SOLR-17376
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Components: clients - java, http2
>Affects Versions: 9.6.1
> Environment: Java version: 21.0.3+9-LTS
> Windows Server 2019
>
>Reporter: Alexander Veit
>Priority: Major
> Labels: memory-bug
> Attachments: Bildschirmfoto vom 2024-07-18 13-23-06.png,
> image-2024-07-23-14-38-17-254.png
>
>
> We use the Solr Java client in a log-running service to update documents in
> an index. As it turns out the Solr client allocates direct buffers that
> accumulate and never get released until JVM restart. After some hundreds of
> thousands instances of java.nio.DirectByteBuffer haven been created the
> system gets stuck and OutOfMemoryErrors occur inside the JVM.
> !Bildschirmfoto vom 2024-07-18 13-23-06.png!
> Unfortunately the JVM does not terminate itself, probably because this is not
> an issue of Java heap memory. Therefore the service could not be restarted
> automatically but requires user interaction.
> Although we have a rather complex scenario, we can almost certainly rule out
> that the leak is due to missing close() calls on the SolrClient instances
> within the JVM. SolrClients are generally reused during the lifetime of the
> JVM.
> The Solr client was created like in the following code
> {code:java}
> SolrClient client = new Http2SolrClient.Builder("my-url").build();{code}
>
> The Solr requests were made like in the following code
> {code:java}
> QueryRequest request = new QueryRequest(params);
>
> request.process(client, "my-collection");{code}
> A heap dump taken from an affected JVM showed the following instance counts:
> {noformat}
> org.apache.solr.client.solrj.impl.Http2SolrClient2
> org.eclipse.jetty.io.LogarithmicArrayByteBufferPool 1
> org.eclipse.jetty.io.MappedByteBufferPool2
> java.nio.DirectByteBuffer 458485{noformat}
>
> The DirectByteBuffers seem to be held alive by a thread-local map.
> !image-2024-07-23-14-38-17-254.png!
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
aparnasuresh85 opened a new pull request, #2584: URL: https://github.com/apache/solr/pull/2584 … https://issues.apache.org/jira/browse/SOLR-X # Description **Summary** This PR introduces `CollectionPropertiesZkStateReader` to manage collection property methods, shifting these responsibilities from `ZkStateReader`. **Purpose** `ZkStateReader` is currently overloaded with tasks, including refreshing and maintaining state for live nodes, collections, cluster state, collection properties, replica properties, aliases, and more. By distributing these responsibilities into specialized classes, we can ensure that each class has a single, well-defined responsibility, thereby reducing complexity. This PR aims to transfer collection property management to `CollectionPropertiesZkStateReader`. - Separation of Concerns: Enhances modularity by isolating logic specific to collection properties. - Improved Maintainability: Simplifies ZkStateReader, making the codebase easier to manage. - Backward Compatibility: `ZkStateReader`'s API remains unchanged, ensuring backward compatibility. # 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. Ensured all existing tests pass. # Checklist Please review the following and check all that apply: - [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
dsmiley commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1688159135
##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##
@@ -246,6 +250,51 @@ public void testAliasHandling() throws Exception {
2, client.query(null,
paramsWithMixedCollectionAndAlias).getResults().getNumFound());
}
+ @Test
+ @LogLevel("org.apache.solr.servlet.HttpSolrCall=INFO")
+ public void testHttpCspPerf() throws Exception {
+
+String COLLECTION = "TEST_COLLECTION";
+CollectionAdminRequest.createCollection(COLLECTION, "conf", 2, 1)
+.process(cluster.getSolrClient());
+cluster.waitForActiveCollection(COLLECTION, 2, 2);
+
+SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my
doc");
+httpBasedCloudSolrClient.add(COLLECTION, doc);
+httpBasedCloudSolrClient.commit(COLLECTION);
+
+Pattern patternWithCollection =
+Pattern.compile(
+
"path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS[^}]*collection=[^&}]+[^}]*\\}");
+
+Pattern patternWithoutCollection =
+Pattern.compile(
+
"path=/admin/collections.*params=\\{[^}]*action=CLUSTERSTATUS(?![^}]*collection=)[^}]*\\}");
+
+LogListener entireClusterStateLogs =
+LogListener.info(HttpSolrCall.class).regex(patternWithoutCollection);
+LogListener collectionClusterStateLogs =
+LogListener.info(HttpSolrCall.class).regex(patternWithCollection);
+
Review Comment:
We also want to ensure that _no other HTTP calls are sent_ (except /select).
That way if we missed something or if somebody adds something later, that we
have this test to ensure that the requests are reasonable.
Similarly do for indexing.
Similarly do for creating the HttpClient itself... albeit maybe defer that
to another JIRA because I think it's fetching the entire cluster status there,
which isn't something we should tackle in this 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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
dsmiley commented on code in PR #2584:
URL: https://github.com/apache/solr/pull/2584#discussion_r1688177033
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##
@@ -719,6 +695,10 @@ private void refreshCollectionList(Watcher watcher) throws
KeeperException, Inte
* changes.
*/
public void registerCloudCollectionsListener(CloudCollectionsListener
cloudCollectionsListener) {
+if (cloudCollectionsListeners.isEmpty()) {
Review Comment:
this is not a pure refactor
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##
@@ -737,6 +717,11 @@ private void notifyCloudCollectionsListeners() {
}
private void notifyCloudCollectionsListeners(boolean notifyIfSame) {
+if (cloudCollectionsListeners.isEmpty()) {
Review Comment:
this is not a pure refactor
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
aparnasuresh85 closed pull request #2584: Delegate Collection Property Management to CollectionPropertiesZkStateReader URL: https://github.com/apache/solr/pull/2584 -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
aparnasuresh85 opened a new pull request, #2585: URL: https://github.com/apache/solr/pull/2585 https://issues.apache.org/jira/browse/SOLR-X # Description **Summary** This PR introduces `CollectionPropertiesZkStateReader` to manage collection property methods, shifting these responsibilities from ZkStateReader. **Purpose** `ZkStateReader` is currently overloaded with tasks, including refreshing and maintaining state for live nodes, collections, cluster state, collection properties, replica properties, aliases, and more. By distributing these responsibilities into specialized classes, we can ensure that each class has a single, well-defined responsibility, thereby reducing complexity. This PR aims to transfer collection property management to `CollectionPropertiesZkStateReader`. Separation of Concerns: Enhances modularity by isolating logic specific to collection properties. Improved Maintainability: Simplifies `ZkStateReader`, making the codebase easier to manage. Backward Compatibility: `ZkStateReader`'s API remains unchanged, ensuring backward compatibility. # 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. Ensure all existing tests pass # Checklist Please review the following and check all that apply: - [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
aparnasuresh85 commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1688240886
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##
@@ -124,7 +124,14 @@ private ClusterState fetchClusterState(
throws SolrServerException, IOException, NotACollectionException {
ModifiableSolrParams params = new ModifiableSolrParams();
if (collection != null) {
+ if (log.isDebugEnabled()) {
+log.debug("Making a call to Solr to fetch cluster state for
collection: {}", collection);
+ }
params.set("collection", collection);
+} else {
+ if (log.isDebugEnabled()) {
+log.debug("Making a call to Solr to fetch entire cluster state");
Review Comment:
Fetching entire cluster state could be prevented from the server side -
maybe as part of another PR/jira 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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Update dependency com.carrotsearch:hppc to v0.10.0 [solr]
cpoerschke merged PR #2512: URL: https://github.com/apache/solr/pull/2512 -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reviewing streaming code and making small clean ups. [solr]
cpoerschke commented on code in PR #2558: URL: https://github.com/apache/solr/pull/2558#discussion_r1688297743 ## solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/eval/CoalesceEvaluatorTest.java: ## @@ -1,7 +1,7 @@ /* * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with - * this work for multitional information regarding copyright ownership. + * this work for additional information regarding copyright ownership. Review Comment: Interesting that the "file has correct license headers" checks didn't pick up on 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reviewing streaming code and making small clean ups. [solr]
epugh commented on code in PR #2558: URL: https://github.com/apache/solr/pull/2558#discussion_r1688300989 ## solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/eval/CoalesceEvaluatorTest.java: ## @@ -1,7 +1,7 @@ /* * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with - * this work for multitional information regarding copyright ownership. + * this work for additional information regarding copyright ownership. Review Comment: does it actually check the internal contents?It looked like a set of cut n paste/ find/replace issues! -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Reviewing streaming code and making small clean ups. [solr]
cpoerschke commented on code in PR #2558:
URL: https://github.com/apache/solr/pull/2558#discussion_r1688301242
##
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/ops/ConcatOperationTest.java:
##
@@ -79,7 +79,7 @@ public void concatMultipleFields() throws Exception {
assertNotNull(tuple.get("fieldABConcat"));
assertEquals("bar-baz", tuple.get("fieldABConcat"));
-// do the same in oposite order
+// do the same in opposite order
Review Comment:
```suggestion
// do the same in reverse order
```
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
dsmiley commented on PR #2585: URL: https://github.com/apache/solr/pull/2585#issuecomment-2245620881 (not ready for review; needs to be re-done) -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] add build badges to README.md [solr]
cpoerschke commented on PR #2546: URL: https://github.com/apache/solr/pull/2546#issuecomment-2245629637 > As an aside, the Gradle dashboard link doesn't yet work for me, could be local issue, not yet looked into. Does it work for anyone else? Just removed it for 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-13350: multi-threaded search: (undocumented) opt-out ability [solr]
cpoerschke merged PR #2570: URL: https://github.com/apache/solr/pull/2570 -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-13350) Explore collector managers for multi-threaded search
[ https://issues.apache.org/jira/browse/SOLR-13350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868111#comment-17868111 ] ASF subversion and git services commented on SOLR-13350: Commit 06950c656f21577db624102b913fb659ef1f0306 in solr's branch refs/heads/main from Christine Poerschke [ https://gitbox.apache.org/repos/asf?p=solr.git;h=06950c656f2 ] SOLR-13350: multi-threaded search: (undocumented) opt-out ability (#2570) On a request level, multiThreaded=false is already possible but for full (node level) opt-out SolrIndexSearcher must pass a null executor to Lucene's IndexSearcher. > Explore collector managers for multi-threaded search > > > Key: SOLR-13350 > URL: https://issues.apache.org/jira/browse/SOLR-13350 > Project: Solr > Issue Type: New Feature >Reporter: Ishan Chattopadhyaya >Assignee: Ishan Chattopadhyaya >Priority: Blocker > Labels: pull-request-available > Fix For: 9.7 > > Attachments: SOLR-13350-pre-PR-2508.patch, SOLR-13350.patch, > SOLR-13350.patch, SOLR-13350.patch > > Time Spent: 13h 10m > Remaining Estimate: 0h > > AFAICT, SolrIndexSearcher can be used only to search all the segments of an > index in series. However, using CollectorManagers, segments can be searched > concurrently and result in reduced latency. Opening this issue to explore the > effectiveness of using CollectorManagers in SolrIndexSearcher from latency > and throughput perspective. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-13350: multi-threaded search: replace cached with fixed threadpool [solr]
cpoerschke commented on code in PR #2508:
URL: https://github.com/apache/solr/pull/2508#discussion_r1688330420
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -445,9 +445,9 @@ public CoreContainer(NodeConfig config, CoresLocator
locator, boolean asyncSolrC
final int indexSearcherExecutorThreads =
cfg.getIndexSearcherExecutorThreads();
if (0 < indexSearcherExecutorThreads) {
this.collectorExecutor =
- ExecutorUtil.newMDCAwareCachedThreadPool(
- indexSearcherExecutorThreads, // thread count
- indexSearcherExecutorThreads * 1000, // queue size
+ ExecutorUtil.newMDCAwareFixedThreadPool(
+ cfg.getIndexSearcherExecutorThreads(), // thread count
+ cfg.getIndexSearcherExecutorThreads() * 1000, // queue size
Review Comment:
oops, hasty merge conflict resolution, i missed this:
```suggestion
indexSearcherExecutorThreads, // thread count
indexSearcherExecutorThreads * 1000, // queue size
```
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-17376) Direct buffer memory leak in Solr Java Client
[
https://issues.apache.org/jira/browse/SOLR-17376?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868121#comment-17868121
]
Alexander Veit commented on SOLR-17376:
---
I don't think that this does apply here.
Almost all of the indexing work is done by the same thread. The increase in
direct memory allocation shown in the graph above is all caused by this one
thread. As far as I can see the mechanism for the allocation of temporary
direct ByteBuffers cannot explain the verly large number of DirectByteBuffers
under these circumstances.
Also, the Jetty HTTP client which is used by the Solr client has an internal
feature which explicitly acquires direct ByteBuffers (see
org.eclipse.jetty.io.ArrayRetainableByteBufferPool), which could very well
explain the observed behaviour.
> Direct buffer memory leak in Solr Java Client
> -
>
> Key: SOLR-17376
> URL: https://issues.apache.org/jira/browse/SOLR-17376
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Components: clients - java, http2
>Affects Versions: 9.6.1
> Environment: Java version: 21.0.3+9-LTS
> Windows Server 2019
>
>Reporter: Alexander Veit
>Priority: Major
> Labels: memory-bug
> Attachments: Bildschirmfoto vom 2024-07-18 13-23-06.png,
> image-2024-07-23-14-38-17-254.png
>
>
> We use the Solr Java client in a log-running service to update documents in
> an index. As it turns out the Solr client allocates direct buffers that
> accumulate and never get released until JVM restart. After some hundreds of
> thousands instances of java.nio.DirectByteBuffer haven been created the
> system gets stuck and OutOfMemoryErrors occur inside the JVM.
> !Bildschirmfoto vom 2024-07-18 13-23-06.png!
> Unfortunately the JVM does not terminate itself, probably because this is not
> an issue of Java heap memory. Therefore the service could not be restarted
> automatically but requires user interaction.
> Although we have a rather complex scenario, we can almost certainly rule out
> that the leak is due to missing close() calls on the SolrClient instances
> within the JVM. SolrClients are generally reused during the lifetime of the
> JVM.
> The Solr client was created like in the following code
> {code:java}
> SolrClient client = new Http2SolrClient.Builder("my-url").build();{code}
>
> The Solr requests were made like in the following code
> {code:java}
> QueryRequest request = new QueryRequest(params);
>
> request.process(client, "my-collection");{code}
> A heap dump taken from an affected JVM showed the following instance counts:
> {noformat}
> org.apache.solr.client.solrj.impl.Http2SolrClient2
> org.eclipse.jetty.io.LogarithmicArrayByteBufferPool 1
> org.eclipse.jetty.io.MappedByteBufferPool2
> java.nio.DirectByteBuffer 458485{noformat}
>
> The DirectByteBuffers seem to be held alive by a thread-local map.
> !image-2024-07-23-14-38-17-254.png!
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17376) Direct buffer memory leak in Solr Java Client
[
https://issues.apache.org/jira/browse/SOLR-17376?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Alexander Veit updated SOLR-17376:
--
Environment:
Java version: 21.0.3+9-LTS
Jetty 10.0.21
Windows Server 2019
was:
Java version: 21.0.3+9-LTS
Windows Server 2019
> Direct buffer memory leak in Solr Java Client
> -
>
> Key: SOLR-17376
> URL: https://issues.apache.org/jira/browse/SOLR-17376
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Components: clients - java, http2
>Affects Versions: 9.6.1
> Environment: Java version: 21.0.3+9-LTS
> Jetty 10.0.21
> Windows Server 2019
>
>Reporter: Alexander Veit
>Priority: Major
> Labels: memory-bug
> Attachments: Bildschirmfoto vom 2024-07-18 13-23-06.png,
> image-2024-07-23-14-38-17-254.png
>
>
> We use the Solr Java client in a log-running service to update documents in
> an index. As it turns out the Solr client allocates direct buffers that
> accumulate and never get released until JVM restart. After some hundreds of
> thousands instances of java.nio.DirectByteBuffer haven been created the
> system gets stuck and OutOfMemoryErrors occur inside the JVM.
> !Bildschirmfoto vom 2024-07-18 13-23-06.png!
> Unfortunately the JVM does not terminate itself, probably because this is not
> an issue of Java heap memory. Therefore the service could not be restarted
> automatically but requires user interaction.
> Although we have a rather complex scenario, we can almost certainly rule out
> that the leak is due to missing close() calls on the SolrClient instances
> within the JVM. SolrClients are generally reused during the lifetime of the
> JVM.
> The Solr client was created like in the following code
> {code:java}
> SolrClient client = new Http2SolrClient.Builder("my-url").build();{code}
>
> The Solr requests were made like in the following code
> {code:java}
> QueryRequest request = new QueryRequest(params);
>
> request.process(client, "my-collection");{code}
> A heap dump taken from an affected JVM showed the following instance counts:
> {noformat}
> org.apache.solr.client.solrj.impl.Http2SolrClient2
> org.eclipse.jetty.io.LogarithmicArrayByteBufferPool 1
> org.eclipse.jetty.io.MappedByteBufferPool2
> java.nio.DirectByteBuffer 458485{noformat}
>
> The DirectByteBuffers seem to be held alive by a thread-local map.
> !image-2024-07-23-14-38-17-254.png!
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-13350: multi-threaded search: replace cached with fixed threadpool [solr]
cpoerschke merged PR #2508: URL: https://github.com/apache/solr/pull/2508 -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-13350) Explore collector managers for multi-threaded search
[ https://issues.apache.org/jira/browse/SOLR-13350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868122#comment-17868122 ] ASF subversion and git services commented on SOLR-13350: Commit dfdbf85a1da491377ab519b025b80d60d4b2d534 in solr's branch refs/heads/main from Christine Poerschke [ https://gitbox.apache.org/repos/asf?p=solr.git;h=dfdbf85a1da ] SOLR-13350: multi-threaded search: replace cached with fixed threadpool (#2508) > Explore collector managers for multi-threaded search > > > Key: SOLR-13350 > URL: https://issues.apache.org/jira/browse/SOLR-13350 > Project: Solr > Issue Type: New Feature >Reporter: Ishan Chattopadhyaya >Assignee: Ishan Chattopadhyaya >Priority: Blocker > Labels: pull-request-available > Fix For: 9.7 > > Attachments: SOLR-13350-pre-PR-2508.patch, SOLR-13350.patch, > SOLR-13350.patch, SOLR-13350.patch > > Time Spent: 13.5h > Remaining Estimate: 0h > > AFAICT, SolrIndexSearcher can be used only to search all the segments of an > index in series. However, using CollectorManagers, segments can be searched > concurrently and result in reduced latency. Opening this issue to explore the > effectiveness of using CollectorManagers in SolrIndexSearcher from latency > and throughput perspective. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-13350) Explore collector managers for multi-threaded search
[ https://issues.apache.org/jira/browse/SOLR-13350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868123#comment-17868123 ] ASF subversion and git services commented on SOLR-13350: Commit cfec121bab2ecfc4c06e20a5533596025ae63d98 in solr's branch refs/heads/branch_9x from Christine Poerschke [ https://gitbox.apache.org/repos/asf?p=solr.git;h=cfec121bab2 ] SOLR-13350: multi-threaded search: replace cached with fixed threadpool (#2508) (cherry picked from commit dfdbf85a1da491377ab519b025b80d60d4b2d534) > Explore collector managers for multi-threaded search > > > Key: SOLR-13350 > URL: https://issues.apache.org/jira/browse/SOLR-13350 > Project: Solr > Issue Type: New Feature >Reporter: Ishan Chattopadhyaya >Assignee: Ishan Chattopadhyaya >Priority: Blocker > Labels: pull-request-available > Fix For: 9.7 > > Attachments: SOLR-13350-pre-PR-2508.patch, SOLR-13350.patch, > SOLR-13350.patch, SOLR-13350.patch > > Time Spent: 13.5h > Remaining Estimate: 0h > > AFAICT, SolrIndexSearcher can be used only to search all the segments of an > index in series. However, using CollectorManagers, segments can be searched > concurrently and result in reduced latency. Opening this issue to explore the > effectiveness of using CollectorManagers in SolrIndexSearcher from latency > and throughput perspective. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17337: Show proper distributed stage id [solr]
cpoerschke commented on code in PR #2526:
URL: https://github.com/apache/solr/pull/2526#discussion_r1688403069
##
solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java:
##
@@ -297,4 +297,12 @@ private void addRequestId(ResponseBuilder rb, String
requestId) {
params.add(CommonParams.REQUEST_ID, requestId);
rb.req.setParams(params);
}
+
+ @Test
+ public void testDistributedStageResolution() throws IOException {
+assertEquals("PARSE_QUERY",
DebugComponent.getDistributedStageName(ResponseBuilder.STAGE_PARSE_QUERY));
Review Comment:
gradlew tidy
```suggestion
assertEquals("PARSE_QUERY",
DebugComponent.getDistributedStageName(ResponseBuilder.STAGE_PARSE_QUERY));
```
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17337) Display all custom distributed stages in debug output
[
https://issues.apache.org/jira/browse/SOLR-17337?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated SOLR-17337:
--
Labels: pull-request-available (was: )
> Display all custom distributed stages in debug output
> -
>
> Key: SOLR-17337
> URL: https://issues.apache.org/jira/browse/SOLR-17337
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: SolrCloud
>Affects Versions: 9.6.1
>Reporter: Torsten Bøgh Köster
>Priority: Major
> Labels: pull-request-available
> Attachments: solr_debug_custom_stage.png
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> When showing {{track}} debug for custom stages in distributed requests, the
> stage id is not translated into a meaningful string. It is displayed as an
> empty string (see attachment). Furthermore, if you have multiple custom
> stages, the details for the most recent stage is displayed. The details for
> other stages is omitted.
> !solr_debug_custom_stage.png!
> We'll provide a GitHub PRs to address both issues.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17337: Show proper distributed stage id [solr]
cpoerschke commented on code in PR #2526:
URL: https://github.com/apache/solr/pull/2526#discussion_r1688403069
##
solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java:
##
@@ -297,4 +297,12 @@ private void addRequestId(ResponseBuilder rb, String
requestId) {
params.add(CommonParams.REQUEST_ID, requestId);
rb.req.setParams(params);
}
+
+ @Test
+ public void testDistributedStageResolution() throws IOException {
+assertEquals("PARSE_QUERY",
DebugComponent.getDistributedStageName(ResponseBuilder.STAGE_PARSE_QUERY));
Review Comment:
gradlew tidy
```suggestion
assertEquals("PARSE_QUERY",
DebugComponent.getDistributedStageName(ResponseBuilder.STAGE_PARSE_QUERY));
```
edit: I just tried to in-browser add the tidy fix here but it won't let me,
likely because the branch is from an org rather than user fork of the repo,
known git issue somewhere.
@tboeghk - would you have a moment to press "commit suggestion" here? 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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Introduce support for Reciprocal Rank Fusion (combining queries) [solr]
cpoerschke commented on code in PR #2489:
URL: https://github.com/apache/solr/pull/2489#discussion_r1688437190
##
solr/solrj/src/java/org/apache/solr/common/params/CombinerParams.java:
##
@@ -0,0 +1,36 @@
+/*
+ * 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.params;
+
+/** Defines the request parameters used by all combiners. */
+public interface CombinerParams {
+ String COMBINER = "combiner";
+
+ String COMBINER_ALGORITHM = COMBINER + ".algorithm";
+
+ String COMBINER_KEYS = COMBINER + ".keys";
Review Comment:
> ... `combine.q` multi-valued ...
In the case of multi-valued parameters, is ordering 'guaranteed' or could
`&combine.q=Foo&combine.q=Bar` turn into `&combine.q=Bar&combine.q=Foo` and if
it did would it matter?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Introduce support for Reciprocal Rank Fusion (combining queries) [solr]
cpoerschke commented on code in PR #2489:
URL: https://github.com/apache/solr/pull/2489#discussion_r1688438487
##
solr/solrj/src/java/org/apache/solr/common/params/CombinerParams.java:
##
@@ -0,0 +1,36 @@
+/*
+ * 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.params;
+
+/** Defines the request parameters used by all combiners. */
+public interface CombinerParams {
+ String COMBINER = "combiner";
+
+ String COMBINER_ALGORITHM = COMBINER + ".algorithm";
+
+ String COMBINER_KEYS = COMBINER + ".keys";
+
+ String RECIPROCAl_RANK_FUSION = "rrf";
Review Comment:
```suggestion
String RECIPROCAL_RANK_FUSION = "rrf";
```
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Introduce support for Reciprocal Rank Fusion (combining queries) [solr]
cpoerschke commented on code in PR #2489:
URL: https://github.com/apache/solr/pull/2489#discussion_r1688453058
##
solr/core/src/java/org/apache/solr/search/combining/CombineQParserPlugin.java:
##
@@ -0,0 +1,167 @@
+/*
+ * 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.search.combining;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import org.apache.lucene.search.BooleanClause.Occur;
+import org.apache.lucene.search.MatchNoDocsQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.solr.common.params.CombinerParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.ExtendedQueryBase;
+import org.apache.solr.search.QParser;
+import org.apache.solr.search.QParserPlugin;
+import org.apache.solr.search.QueryCommand;
+import org.apache.solr.search.QueryParsing;
+import org.apache.solr.search.QueryResult;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SyntaxError;
+
+public class CombineQParserPlugin extends QParserPlugin {
+ public static final String NAME = "combine";
+
+ @Override
+ public QParser createParser(
+ String qstr, SolrParams localParams, SolrParams params, SolrQueryRequest
req) {
+return new CombineQParser(qstr, localParams, params, req);
+ }
+
+ static class CombineQParser extends QParser {
+
+private List unparsedQueries;
+private QueriesCombiner queriesCombiningStrategy;
+private List queryParsers;
+private List queries;
+
+public CombineQParser(
+String qstr, SolrParams localParams, SolrParams params,
SolrQueryRequest req) {
+ super(qstr, localParams, params, req);
+}
+
+@Override
+public Query parse() throws SyntaxError {
+
+ var queriesToCombineKeys = localParams.getParams("keys");
+ if (queriesToCombineKeys == null) {
+queriesToCombineKeys = params.getParams(CombinerParams.COMBINER_KEYS);
+ }
+
+ unparsedQueries = new ArrayList<>(queriesToCombineKeys.length);
+ queryParsers = new ArrayList<>(queriesToCombineKeys.length);
+ queries = new ArrayList<>(queriesToCombineKeys.length);
+
+ // nocommit blend localParams and params without needless suffix in local
+ var blendParams = SolrParams.wrapDefaults(localParams, params);
+ queriesCombiningStrategy =
QueriesCombiner.getImplementation(blendParams);
+
+ String defType = blendParams.get(QueryParsing.DEFTYPE, DEFAULT_QTYPE);
Review Comment:
Maybe could support different `defType` for different keys, though bit of a
niche thing that perhaps.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Introduce support for Reciprocal Rank Fusion (combining queries) [solr]
cpoerschke commented on code in PR #2489:
URL: https://github.com/apache/solr/pull/2489#discussion_r1688454713
##
solr/core/src/java/org/apache/solr/search/combining/QueriesCombiner.java:
##
@@ -0,0 +1,93 @@
+/*
+ * 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.search.combining;
+
+import static
org.apache.solr.common.params.CombinerParams.RECIPROCAl_RANK_FUSION;
+
+import java.io.IOException;
+import java.util.List;
+import org.apache.lucene.search.Explanation;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.CombinerParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.schema.IndexSchema;
+import org.apache.solr.search.DocList;
+import org.apache.solr.search.QueryResult;
+import org.apache.solr.search.SolrIndexSearcher;
+
+/**
+ * Combining considers two or more query rankedLists: resultA, resultB ...
+ * For a given query, each query result is a ranked list of documents La =
(a1,a2,...), Lb = (b1,
+ * b2, ...)...
+ * A combining algorithm creates a unique ranked list I = (i1, i2, ...).
+ * This list is created by combining elements from the lists la and lb as
described by the
+ * implementation algorithm.
+ * since @Version 9.7 Used by {@link
org.apache.solr.handler.component.QueryComponent}
+ */
+public abstract class QueriesCombiner {
+
+ protected int upTo;
+
+ public QueriesCombiner(SolrParams requestParams) {
+this.upTo =
+requestParams.getInt(CombinerParams.COMBINER_UP_TO,
CombinerParams.COMBINER_UP_TO_DEFAULT);
+ }
+
+ public abstract QueryResult combine(QueryResult[] rankedLists);
+
+ protected QueryResult initCombinedResult(QueryResult[] rankedLists) {
+QueryResult combinedRankedList = new QueryResult();
+boolean partialResults = false;
+for (QueryResult result : rankedLists) {
+ partialResults |= result.isPartialResults();
+}
+combinedRankedList.setPartialResults(partialResults);
+
+boolean segmentTerminatedEarly = false;
+for (QueryResult result : rankedLists) {
+ if (result.getSegmentTerminatedEarly() != null) {
+segmentTerminatedEarly |= result.getSegmentTerminatedEarly();
+ }
+}
+combinedRankedList.setSegmentTerminatedEarly(segmentTerminatedEarly);
+
+combinedRankedList.setNextCursorMark(rankedLists[0].getNextCursorMark());
Review Comment:
Not sure about picking the first list here and/or what does use of a cursor
mark mean when combining queries. Alternative might be to just disallow cursor
marks when combining queries.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Created] (SOLR-17377) ClusterSingleton defined in solr.xml cannot be loaded from a module
Pierre Salagnac created SOLR-17377:
--
Summary: ClusterSingleton defined in solr.xml cannot be loaded
from a module
Key: SOLR-17377
URL: https://issues.apache.org/jira/browse/SOLR-17377
Project: Solr
Issue Type: Bug
Security Level: Public (Default Security Level. Issues are Public)
Affects Versions: 9.6, 9.5
Reporter: Pierre Salagnac
Following SOLR-17096, we can declare a custom cluster singleton in {{solr.xml}}
file.
There is an early check in class {{SolrXmlConfig}} to ensure the plugin
actually implements interface {{{}ClusterSingleton{}}}. This check is broken
for modules and fails for any class which is defined as an external plugin, and
not in the Solr code itself because of a {{{}ClassNotFoundException{}}}.
My understanding is at this point, we don't have the _extended_ class loader
that includes plugins yet.
{code:java}
org.apache.solr.common.SolrException: Error loading class 'my.package.MyClass'
at
org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:550)
at
org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:471)
at
org.apache.solr.core.SolrXmlConfig.lambda$getClusterSingletonPluginInfos$10(Unknown
Source)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at
org.apache.solr.core.SolrXmlConfig.getClusterSingletonPluginInfos(SolrXmlConfig.java:714)
at
org.apache.solr.core.SolrXmlConfig.getClusterPlugins(SolrXmlConfig.java:668)
...
Caused by: java.lang.ClassNotFoundException: my.package.MyClass
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
{code}
With the code commented, the singleton is loaded with no error later. The issue
is only with this early check itself, and not when instantiating and starting
the singleton.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17377) ClusterSingleton defined in solr.xml cannot be loaded from a module
[
https://issues.apache.org/jira/browse/SOLR-17377?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Pierre Salagnac updated SOLR-17377:
---
Description:
Following SOLR-17096, we can declare a custom cluster singleton in {{solr.xml}}
file.
There is an early check in class {{SolrXmlConfig}} to ensure the plugin
actually implements interface {{{}ClusterSingleton{}}}. This check is broken
for modules and fails for any class which is defined as an external plugin, and
not in the Solr code itself because of a {{{}ClassNotFoundException{}}}.
My understanding is at this point, we don't have the _extended_ class loader
that includes plugins yet.
{code:java}
org.apache.solr.common.SolrException: Error loading class 'my.package.MyClass'
at
org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:550)
at
org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:471)
at
org.apache.solr.core.SolrXmlConfig.lambda$getClusterSingletonPluginInfos$10(Unknown
Source)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at
org.apache.solr.core.SolrXmlConfig.getClusterSingletonPluginInfos(SolrXmlConfig.java:714)
at
org.apache.solr.core.SolrXmlConfig.getClusterPlugins(SolrXmlConfig.java:668)
...
Caused by: java.lang.ClassNotFoundException: my.package.MyClass
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
{code}
With the code of the check commented, the singleton is loaded with no error
later. The issue is only with this early check itself, and not when
instantiating and starting the singleton.
was:
Following SOLR-17096, we can declare a custom cluster singleton in {{solr.xml}}
file.
There is an early check in class {{SolrXmlConfig}} to ensure the plugin
actually implements interface {{{}ClusterSingleton{}}}. This check is broken
for modules and fails for any class which is defined as an external plugin, and
not in the Solr code itself because of a {{{}ClassNotFoundException{}}}.
My understanding is at this point, we don't have the _extended_ class loader
that includes plugins yet.
{code:java}
org.apache.solr.common.SolrException: Error loading class 'my.package.MyClass'
at
org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:550)
at
org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:471)
at
org.apache.solr.core.SolrXmlConfig.lambda$getClusterSingletonPluginInfos$10(Unknown
Source)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at
org.apache.solr.core.SolrXmlConfig.getClusterSingletonPluginInfos(SolrXmlConfig.java:714)
at
org.apache.solr.core.SolrXmlConfig.getClusterPlugins(SolrXmlConfig.java:668)
...
Caused by: java.lang.ClassNotFoundException: my.package.MyClass
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:476)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
{code}
With the code commented, the singleton is loaded with no error later. The issue
is only with this early check itself, and not when instantiating and starting
the singleton.
> ClusterSingleton defined in solr.xml cannot be loaded from a module
> ---
>
> Key: SOLR-17377
> URL: https://issues.apache.org/jira/browse/SOLR-17377
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
>Affects Versions: 9.5, 9.6
>Reporter: Pierre Salagnac
>Priority: Major
>
> Following SOLR-17096, we can declare a custom cluster singleton in
> {{solr.xml}} file.
> There is an early check in class {{SolrXmlConfig}} to ensure the plugin
> actually implements interface {{{}ClusterSingleton{}}}. This check is broken
> for modules and fails for any class which is defined as an external plugin,
> and not in the Solr code itself because of a {{{}ClassNotFoundException{}}}.
> My understanding is at this point, we don't have the _extended_ class loader
> that includes plugins yet.
> {code:java}
> org.apache.solr.common.SolrException: Error loading class
> 'my.package.MyClass'
> at
> org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:550)
> at
> org.apache.solr.core.SolrResourceLoader.findClass(SolrResourceLoader.java:471)
> at
> org.apache.solr.core.SolrXmlConfig.lambda$getClusterSingletonPluginInfos$10(Unknown
> Source)
> at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
> at
> org.apache.solr.core.SolrXmlConfig.getClusterSingletonPluginInfos(SolrXmlConfig.java:714)
> at
> org.apache.solr.core.SolrXmlConfig.getClusterPlugins(SolrXm
Re: [PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
dsmiley commented on code in PR #2585:
URL: https://github.com/apache/solr/pull/2585#discussion_r1688485078
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##
@@ -1191,130 +1166,27 @@ private void loadClusterProperties() {
}
/**
- * Get collection properties for a given collection. If the collection is
watched, simply return
- * it from the cache, otherwise fetch it directly from zookeeper. This is a
convenience for {@code
- * getCollectionProperties(collection,0)}
+ * Retrieves the properties for a specific collection.
*
- * @param collection the collection for which properties are desired
- * @return a map representing the key/value properties for the collection.
+ * This method is delegated to {@link
+ * CollectionPropertiesZkStateReader#getCollectionProperties(String,long)}.
Review Comment:
Why say this? This is an implementation detail.
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -0,0 +1,403 @@
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptyMap;
+
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.solr.common.SolrCloseable;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader.CollectionWatch;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.ObjectReleaseTracker;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class CollectionPropertiesZkStateReader implements SolrCloseable {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private volatile boolean closed = false;
+
+ private final SolrZkClient zkClient;
+
+ /** Collection properties being actively watched */
+ private final ConcurrentHashMap
watchedCollectionProps =
+ new ConcurrentHashMap<>();
+
+ /**
+ * Manages ZooKeeper watchers for each collection. These watchers monitor
changes to the
+ * properties of the collection in ZooKeeper. When a change is detected in
ZooKeeper, the watcher
+ * triggers an update, which then notifies the relevant
"collectionPropsObserver".
+ */
+ private final ConcurrentHashMap
collectionPropsWatchers =
+ new ConcurrentHashMap<>();
+
+ /**
+ * Manages a list of observers (listeners) for each collection. These
observers need to be
+ * notified when the properties of the collection change. When a
collection's properties change,
+ * all registered observers for that collection are notified by a
"collectionPropWatcher".
+ */
+ private ConcurrentHashMap>
+ collectionPropsObservers = new ConcurrentHashMap<>();
+
+ /** Used to submit notifications to Collection Properties watchers in order
*/
+ private final ExecutorService collectionPropsNotifications =
+ ExecutorUtil.newMDCAwareSingleThreadExecutor(
+ new SolrNamedThreadFactory("collectionPropsNotifications"));
+
+ private final ExecutorService notifications =
+ ExecutorUtil.newMDCAwareCachedThreadPool("cachecleaner");
+
+ // only kept to identify if the cleaner has already been started.
+ private Future collectionPropsCacheCleaner;
+
+ public CollectionPropertiesZkStateReader(SolrZkClient zkClient) {
+this.zkClient = zkClient;
+assert ObjectReleaseTracker.track(this);
+ }
+
+ /**
+ * Get and cache collection properties for a given collection. If the
collection is watched, or
+ * still cached simply return it from the cache, otherwise fetch it directly
from zookeeper and
+ * retain the value for at least cacheForMillis milliseconds. Cached
properties are watched in
+ * zookeeper and updated automatically. This version of {@code
getCollectionProperties} should be
+ * used when properties need to be consulted frequently in the absence of an
active {@link
+ * CollectionPropsWatcher}.
+ *
+ * @param collection The collection for which properties are desired
+ * @param cacheForMillis The minimum number of milliseconds to maintain a
cache for the specified
+ * collection's properties. Setting a {@code CollectionPropsWatcher}
will override this value
+ * and retain the cache for the life of the watcher. A lack of changes
in zookeeper may allow
+ * the caching to remain for a greate
Re: [PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
dsmiley commented on PR #2585: URL: https://github.com/apache/solr/pull/2585#issuecomment-2245945375 ZkStateReader has been getting too large; I like this change! @gus-asf @tflobbe , I'm requesting your feedback on this pure refactoring PR (I wish the title used the word "refactoring" to emphasize this) by my colleague. The previous attempts were wrong but this time it's a clean-room change here of just moving code around. It's not intended to have any observed effect. I will merge this in 48 hours if I get no +1 but I may merge sooner with a +1. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Delegate Collection Property Management to CollectionPropertiesZkStateReader [solr]
dsmiley commented on code in PR #2585:
URL: https://github.com/apache/solr/pull/2585#discussion_r1688516826
##
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##
@@ -0,0 +1,403 @@
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptyMap;
+
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.solr.common.SolrCloseable;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader.CollectionWatch;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.ObjectReleaseTracker;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class CollectionPropertiesZkStateReader implements SolrCloseable {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private volatile boolean closed = false;
+
+ private final SolrZkClient zkClient;
+
+ /** Collection properties being actively watched */
+ private final ConcurrentHashMap
watchedCollectionProps =
+ new ConcurrentHashMap<>();
+
+ /**
+ * Manages ZooKeeper watchers for each collection. These watchers monitor
changes to the
+ * properties of the collection in ZooKeeper. When a change is detected in
ZooKeeper, the watcher
+ * triggers an update, which then notifies the relevant
"collectionPropsObserver".
+ */
+ private final ConcurrentHashMap
collectionPropsWatchers =
+ new ConcurrentHashMap<>();
+
+ /**
+ * Manages a list of observers (listeners) for each collection. These
observers need to be
+ * notified when the properties of the collection change. When a
collection's properties change,
+ * all registered observers for that collection are notified by a
"collectionPropWatcher".
+ */
+ private ConcurrentHashMap>
+ collectionPropsObservers = new ConcurrentHashMap<>();
+
+ /** Used to submit notifications to Collection Properties watchers in order
*/
+ private final ExecutorService collectionPropsNotifications =
+ ExecutorUtil.newMDCAwareSingleThreadExecutor(
+ new SolrNamedThreadFactory("collectionPropsNotifications"));
+
+ private final ExecutorService notifications =
+ ExecutorUtil.newMDCAwareCachedThreadPool("cachecleaner");
+
+ // only kept to identify if the cleaner has already been started.
+ private Future collectionPropsCacheCleaner;
+
+ public CollectionPropertiesZkStateReader(SolrZkClient zkClient) {
+this.zkClient = zkClient;
+assert ObjectReleaseTracker.track(this);
+ }
+
+ /**
+ * Get and cache collection properties for a given collection. If the
collection is watched, or
+ * still cached simply return it from the cache, otherwise fetch it directly
from zookeeper and
+ * retain the value for at least cacheForMillis milliseconds. Cached
properties are watched in
+ * zookeeper and updated automatically. This version of {@code
getCollectionProperties} should be
+ * used when properties need to be consulted frequently in the absence of an
active {@link
+ * CollectionPropsWatcher}.
+ *
+ * @param collection The collection for which properties are desired
+ * @param cacheForMillis The minimum number of milliseconds to maintain a
cache for the specified
+ * collection's properties. Setting a {@code CollectionPropsWatcher}
will override this value
+ * and retain the cache for the life of the watcher. A lack of changes
in zookeeper may allow
+ * the caching to remain for a greater duration up to the cycle time of
{@code CacheCleaner}.
+ * Passing zero for this value will explicitly remove the cached copy if
and only if it is due
+ * to expire and no watch exists. Any positive value will extend the
expiration time if
+ * required.
+ * @return a map representing the key/value properties for the collection.
+ */
+ public Map getCollectionProperties(final String collection,
long cacheForMillis) {
+synchronized (watchedCollectionProps) { // synchronized on the specific
collection
+ Watcher watcher = null;
+ if (cacheForMillis > 0) {
+watcher =
+collectionPropsWatchers.compute(
+collection,
+(c, w) ->
+w == null ? new PropsWatcher(c, cacheForMillis) :
w.renew(cacheForMillis));
+ }
+
Re: [PR] SOLR-14985 Address perf issues to switch CloudSolrClient to HttpCluste… [solr]
dsmiley commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1688601156
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##
@@ -1213,10 +1193,6 @@ protected DocCollection getDocCollection(String
collection, Integer expectedVers
// no such collection exists
return null;
}
-if (!ref.isLazilyLoaded()) {
Review Comment:
BTW, I uncovered that this lazy check was introduced in SOLR-6521 as part of
a performance improvement relating to ensuring that concurrent cache lookups of
an expired entry don't redundantly fetch it in parallel.
> I believe ZK CSP always has its DocCollections cached in CSP since its
works with Lazy refs
I confirmed this as well in a debugger. It's because a ZkStateReader
created *for* ZK CSP doesn't watch the state of any single collection (albeit
it does watch the parent /collections path), whereas a ZkStateReader created in
ZkController (i.e. on the server) does watch individual collections for local
replicas.
Two questions upon us: do we
* (A) remove !isLazilyLoaded short-circuit. Basically all users use ZK CSP
and this doesn't trigger for that. We fixed the HTTP CSP performance
interaction here so it can be removed.
* (B) keep the short-circuit, and make HTTP CSP return a CollectionRef that
is lazy
I actually think BOTH :-)
Just (A) is tempting; it "works" and addresses the obvious perf regression.
However, note the comment "we MUST try to get a new version" and "this is a
call to ZK" some lines below. This code is written around the idea that the
potentially expensive part is down there where we hold a lock (SOLR-6521).
Code archeology here was helpful to uncover the intent. If we don't make it
lazy, we miss the point and then SOLR-6521 accomplishes nothing for the HTTP
CSP scenario. We should go as far as asserting that the ref.isLazilyLoaded and
document getCollectionRef that it _must_ return "lazy" references.
Thinking of that lock and SOLR-6521, we could disregard the lazy/non-lazy
issue and move `ClusterState.CollectionRef ref = getCollectionRef(collection);`
from before the lock to right down into the lock before calling `get()` on it.
Then it doesn't matter if it's lazy or not; the real lookup (talking to either
ZK or HTTP Solr) will be in there, which is the intent of SOLR-6521. I played
with this locally and tests pass. Lets do 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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Introduce support for Reciprocal Rank Fusion (combining queries) [solr]
dsmiley commented on code in PR #2489:
URL: https://github.com/apache/solr/pull/2489#discussion_r1688607183
##
solr/solrj/src/java/org/apache/solr/common/params/CombinerParams.java:
##
@@ -0,0 +1,36 @@
+/*
+ * 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.params;
+
+/** Defines the request parameters used by all combiners. */
+public interface CombinerParams {
+ String COMBINER = "combiner";
+
+ String COMBINER_ALGORITHM = COMBINER + ".algorithm";
+
+ String COMBINER_KEYS = COMBINER + ".keys";
Review Comment:
AFAIK it's guaranteed; we use a List not a Set intentionally
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Introduce support for Reciprocal Rank Fusion (combining queries) [solr]
dsmiley commented on PR #2489: URL: https://github.com/apache/solr/pull/2489#issuecomment-2246125113 @cpoerschke see the parent JIRA issue for the real state of this controvertial 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Created] (SOLR-17378) Add a RequestHandler metric for "number of outstanding concurrent requests"
Michael Gibney created SOLR-17378: - Summary: Add a RequestHandler metric for "number of outstanding concurrent requests" Key: SOLR-17378 URL: https://issues.apache.org/jira/browse/SOLR-17378 Project: Solr Issue Type: Improvement Security Level: Public (Default Security Level. Issues are Public) Reporter: Michael Gibney "number of outstanding concurrent requests" is an important metric to track, and can have a significant impact on resource usage and throughput. Existing metrics provide a window into request count and request latency, but neither of these is sufficient to supply the desired "concurrency" metric. Leveraging request latency and completed request timestamp, it's possible to retroactively compute outstanding concurrent requests, but existing metrics are incapable of capturing this information directly or presenting it in an aggregate format. In addition to the implications for performance, it is important to have a window into request concurrency to complement the solr rate limiting feature, whose "slot acquisition" design really limits request concurrency, _not_ (as the name implies) request count/throughput. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-14673) Add CLI for Streaming Expressions
[
https://issues.apache.org/jira/browse/SOLR-14673?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated SOLR-14673:
--
Labels: pull-request-available (was: )
> Add CLI for Streaming Expressions
> -
>
> Key: SOLR-14673
> URL: https://issues.apache.org/jira/browse/SOLR-14673
> Project: Solr
> Issue Type: New Feature
> Components: streaming expressions
>Reporter: Joel Bernstein
>Assignee: Eric Pugh
>Priority: Major
> Labels: pull-request-available
> Attachments: SOLR-14673.patch, SOLR-14673.patch, SOLR-14673.patch,
> SOLR-14673.patch, SOLR-14673.patch
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> This ticket will provide a simple CLI that will run a Streaming Expression
> from the command line and return the results as a delimited result set. This
> will allow Streaming Expressions to be used from the command line to extract
> data as well as load data into Solr.
> Sample syntax:
> {code:java}
> bin/expr expr_file{code}
> This will run the expression in _expr_file_.
> Output will be to standard out as delimited records.
> *Standard-In, Standard-out and Pipe Composition.*
> The CLI can read from *standard-in* and write to *standard-out* in delimited
> records. This allows Streaming Expressions to be composed using unix pipes
> with other command line tools and other streaming expressions. Example syntax:
> {code:java}
> cat data.csv | ./bin/expr load.expr {code}
> The load.expr file uses the *stndin()* function to read from standard-in and
> load date to Solr. Here is a sample load.expr
> {code:java}
> let(cli-zkhost="localhost:9983",
>
> update(collection1,
>parseCSV(stndin()))
> {code}
> In the example above the *let* expression is used to set the *cli-zkhost* and
> then run the Streaming Expression:
> {code:java}
> update(collection1,
>parseCSV(stndin()){code}
> The *stndin* function reads the data from standard-in. The *parseCSV*
> function parses the lines into tuples and the *update* function sends the
> data to collection1 for indexing.
>
> *Querying Solr and Pipe Composition*
> The CLI can also be used to query Solr and pipe the result to other
> applications on the command line. This can automate activities like alerting,
> data backup, replication etc...
>
>
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-15831: Refactor bin/solr and bin/solr.cmd to delegate args parsing and usage help to SolrCLI [solr]
epugh commented on PR #2580: URL: https://github.com/apache/solr/pull/2580#issuecomment-2246262293 @janhoy what do you think about me merging 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-14673: Add bin/solr stream CLI [solr]
epugh commented on PR #2479: URL: https://github.com/apache/solr/pull/2479#issuecomment-2246259400 Okay, I think this is ready for review! I've added some docs.. I especially liked being able to cat some local data right into a Solr collection! ``` cat example/exampledocs/books.csv | bin/solr stream -e local 'update(gettingstarted,parseCSV(stdin()))' ``` -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] [SOLR-17368] TestPrometheusResponseWriter redesign [solr]
mlbiscoc opened a new pull request, #2586: URL: https://github.com/apache/solr/pull/2586 https://issues.apache.org/jira/browse/SOLR-17368 # Description Please provide a short description of the changes you're making with this pull request. The `TestPrometheusResponseWriter` was failing 100% of Jenkins builds due to leaking metrics from other tests running on the same JVM. This PR will redesigns the `TestPrometheusResponseWriter` to only test the structure of the output from the PrometheusResponseWriter and not the output of every metric that exists in Solr. # Solution Please provide a short description of the approach taken to implement your solution. Removed the `solr-prometheus-output.txt` output entirely. Filter out comments from the output (i.e `#` lines) and make an assertion that all metric names must start with `solr_metrics_*` and values must be a valid Prometheus accepted value `floating point number, NaN, +Inf, or -Inf` per Prometheus exposition format documentation. Also created `testPrometheusDummyOutput` that registers a dummy metrics for each registry and asserts against it's expected value. Because these are non-existent dummy values, any leaking metrics from other metrics should not override these values. # Tests Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem. `testPrometheusStructureOutput` and `testPrometheusDummyOutput` # Checklist Please review the following and check all that apply: - [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17368) TestPrometheusResponseWriter redesign
[ https://issues.apache.org/jira/browse/SOLR-17368?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] ASF GitHub Bot updated SOLR-17368: -- Labels: pull-request-available (was: ) > TestPrometheusResponseWriter redesign > - > > Key: SOLR-17368 > URL: https://issues.apache.org/jira/browse/SOLR-17368 > Project: Solr > Issue Type: Test > Security Level: Public(Default Security Level. Issues are Public) >Reporter: Chris M. Hostetter >Priority: Major > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > TestPrometheusResponseWriter currently fails in 100% of all jenkins builds > due to how it is designed, and depending on what other tests may run before > it in the same JVM. > This problem only affects the test, not the functionality of the underlying > code. > See SOLR-10654 for background discussions of the problems with this test, and > options for improving it's design relative to it's purpose. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17285: Move RemoteSolrException to SolrClient in v10 [solr]
moistbeans23 opened a new pull request, #2587: URL: https://github.com/apache/solr/pull/2587 https://issues.apache.org/jira/browse/SOLR-17285 # Description Moved RemoteSolrException to SolrClient from BaseHttpSolrClient # Solution Moved RemoteSolrException class to SolrClient. Manually changed import statements from BaseHttpSolrClient to SolrClient if is not needed anymore for RemoteSolrException. Changed BaseHttpSolrClient.RemoteSolrException to SolrClient.RemoteSolrException # Tests [OUTPUT-org.apache.solr.cloud.HttpPartitionOnCommitTest.txt](https://github.com/user-attachments/files/16354132/OUTPUT-org.apache.solr.cloud.HttpPartitionOnCommitTest.txt) [OUTPUT-org.apache.solr.cloud.ReplaceNodeTest.txt](https://github.com/user-attachments/files/16354138/OUTPUT-org.apache.solr.cloud.ReplaceNodeTest.txt) [OUTPUT-org.apache.solr.handler.TestSolrConfigHandlerConcurrent.txt](https://github.com/user-attachments/files/16354141/OUTPUT-org.apache.solr.handler.TestSolrConfigHandlerConcurrent.txt) [OUTPUT-org.apache.solr.pkg.TestPackages.txt](https://github.com/user-attachments/files/16354145/OUTPUT-org.apache.solr.pkg.TestPackages.txt) I ran ./gradlew dev. It worked fine `./gradlew check` did not work. It said there were failing tests on unrelated files # Checklist Please review the following and check all that apply: - [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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) - [x] I have developed this patch against the `main` branch. - [x] I have run `./gradlew check`. - [x] 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17285) Move RemoteSolrException to SolrClient in v10
[ https://issues.apache.org/jira/browse/SOLR-17285?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] ASF GitHub Bot updated SOLR-17285: -- Labels: newdev pull-request-available (was: newdev) > Move RemoteSolrException to SolrClient in v10 > - > > Key: SOLR-17285 > URL: https://issues.apache.org/jira/browse/SOLR-17285 > Project: Solr > Issue Type: Sub-task > Components: SolrJ >Reporter: David Smiley >Priority: Major > Labels: newdev, pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > RemoteSolrException lives in BaseHttpSolrClient. BaseHttpSolrClient should > be deprecated; it's sort of replaced by HttpSolrClientBase. Even though this > exception is only for Http, SolrClient is a decent parent class. Or make top > level. > To make this transition from 9x to 10x better, we could simply add new > classes without removing the old ones in 9x. The old can subclass the new. > Eventually all of BaseHttpSolrClient will be removed. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
malliaridis commented on code in PR #2577: URL: https://github.com/apache/solr/pull/2577#discussion_r1688758777 ## solr/bin/solr.cmd: ## @@ -292,7 +292,6 @@ IF "%FIRST_ARG%"=="-help" goto run_solrcli IF "%FIRST_ARG%"=="-usage" goto run_solrcli IF "%FIRST_ARG%"=="-h" goto run_solrcli IF "%FIRST_ARG%"=="--help" goto run_solrcli -IF "%FIRST_ARG%"=="-help" goto run_solrcli Review Comment: Line 295 is a duplication of 291, which is why I removed it. Should probably not have an effect? -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
malliaridis commented on code in PR #2577: URL: https://github.com/apache/solr/pull/2577#discussion_r1688759945 ## solr/bin/solr.cmd: ## @@ -1525,10 +1524,10 @@ IF "!ZK_OP!"=="" ( set CONNECTION_PARAMS="" IF "!ZK_OP!"=="" ( - set CONNECTION_PARAMS="-solrUrl !ZK_SOLR_URL!" + set CONNECTION_PARAMS="--solr-url !ZK_SOLR_URL!" Review Comment: For deprecation reasons I assume this should be updated to the new format, correct? -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
malliaridis commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1688763185
##
solr/packaging/test/test_adminconsole_urls.bats:
##
@@ -25,7 +25,7 @@ teardown() {
# save a snapshot of SOLR_HOME for failed tests
save_home_on_failure
- solr stop -all >/dev/null 2>&1
+ solr stop --all >/dev/null 2>&1
Review Comment:
I think adding tests in addition that checks that both still work is a good
idea.
As far as I understand, the function "teardown" is responsible for test
cleanup, so I guess it can be updated without problems? Or is there a chance
that the test will be executed with a version that does not support `--all`?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
malliaridis commented on code in PR #2577: URL: https://github.com/apache/solr/pull/2577#discussion_r1688758777 ## solr/bin/solr.cmd: ## @@ -292,7 +292,6 @@ IF "%FIRST_ARG%"=="-help" goto run_solrcli IF "%FIRST_ARG%"=="-usage" goto run_solrcli IF "%FIRST_ARG%"=="-h" goto run_solrcli IF "%FIRST_ARG%"=="--help" goto run_solrcli -IF "%FIRST_ARG%"=="-help" goto run_solrcli Review Comment: Line 295 is a duplication of 291, which is why I removed it. Should probably not have an effect? EDIT: What exactly do you mean with "special case"? I think I don't interpret correctly your comment. 😅 -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17285: Move RemoteSolrException to SolrClient in v10 [solr]
dsmiley commented on PR #2587: URL: https://github.com/apache/solr/pull/2587#issuecomment-2246403844 I am looking at BaseHttpSolrClient again, and I see that RemoteExecutionException is *also* something that'll need to be moved. It's not used in many places. I think it's reasonable to include that in the scope of this JIRA issue and PR; WDYT? But I don't love the name "RemoteExecutionException" and I'm not even convinced it needs to exist; like couldn't RemoteSolrException take a payload? Publishing it to SolrClient would give it undeserved visibility IMO. Hmmm. On the other hand, based on how it's used, this is so internal that maybe it should simply move to HttpSolrClientBase (which is the successor to BaseHttpSolrClient). I welcome your thoughts. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-17368] TestPrometheusResponseWriter redesign [solr]
dsmiley commented on code in PR #2586:
URL: https://github.com/apache/solr/pull/2586#discussion_r1688787862
##
solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java:
##
@@ -34,57 +38,104 @@
import org.junit.ClassRule;
import org.junit.Test;
-/** Tests the {@link PrometheusResponseWriter} behavior */
-@LuceneTestCase.AwaitsFix(bugUrl =
"https://issues.apache.org/jira/browse/SOLR-17368";)
public class TestPrometheusResponseWriter extends SolrTestCaseJ4 {
-
@ClassRule public static SolrJettyTestRule solrClientTestRule = new
SolrJettyTestRule();
public static JettySolrRunner jetty;
+ public static final List VALID_PROMETHEUS_VALUES =
Arrays.asList("NaN", "+Inf", "-Inf");
@BeforeClass
public static void beforeClass() throws Exception {
solrClientTestRule.startSolr(LuceneTestCase.createTempDir());
jetty = solrClientTestRule.getJetty();
solrClientTestRule.newCollection().withConfigSet(ExternalPaths.DEFAULT_CONFIGSET).create();
jetty.getCoreContainer().waitForLoadingCoresToFinish(3);
Review Comment:
can call solrClientTestRule.getCoreContainer() instead and same below; maybe
put in a var "cc" (a popular abbreviation in Solr for this super-common class)
##
solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java:
##
@@ -34,57 +38,104 @@
import org.junit.ClassRule;
import org.junit.Test;
-/** Tests the {@link PrometheusResponseWriter} behavior */
-@LuceneTestCase.AwaitsFix(bugUrl =
"https://issues.apache.org/jira/browse/SOLR-17368";)
public class TestPrometheusResponseWriter extends SolrTestCaseJ4 {
-
@ClassRule public static SolrJettyTestRule solrClientTestRule = new
SolrJettyTestRule();
public static JettySolrRunner jetty;
Review Comment:
I think this field serves no purpose; we can get a jetty if needed from the
rule on the fly. Static fields in tests can have a maintenance burden to
ensure they get null'ed.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-17368] TestPrometheusResponseWriter redesign [solr]
dsmiley commented on PR #2586: URL: https://github.com/apache/solr/pull/2586#issuecomment-2246415444 > any leaking metrics from other metrics should not override these values. That's dodging an issue that should be investigated as it might interfere with another metrics based test I suppose. Any way; progress not perfection. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-17378) Add a RequestHandler metric for "number of outstanding concurrent requests"
[ https://issues.apache.org/jira/browse/SOLR-17378?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868199#comment-17868199 ] David Smiley commented on SOLR-17378: - Sounds useful! > Add a RequestHandler metric for "number of outstanding concurrent requests" > --- > > Key: SOLR-17378 > URL: https://issues.apache.org/jira/browse/SOLR-17378 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) >Reporter: Michael Gibney >Priority: Minor > > "number of outstanding concurrent requests" is an important metric to track, > and can have a significant impact on resource usage and throughput. Existing > metrics provide a window into request count and request latency, but neither > of these is sufficient to supply the desired "concurrency" metric. > Leveraging request latency and completed request timestamp, it's possible to > retroactively compute outstanding concurrent requests, but existing metrics > are incapable of capturing this information directly or presenting it in an > aggregate format. > In addition to the implications for performance, it is important to have a > window into request concurrency to complement the solr rate limiting feature, > whose "slot acquisition" design really limits request concurrency, _not_ (as > the name implies) request count/throughput. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-15831: Refactor bin/solr and bin/solr.cmd to delegate args parsing and usage help to SolrCLI [solr]
janhoy commented on PR #2580: URL: https://github.com/apache/solr/pull/2580#issuecomment-2246635757 Sure, great steps in right direction -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
