[jira] [Commented] (SOLR-17630) Add CloudSolrClient instance for a Solr node
[
https://issues.apache.org/jira/browse/SOLR-17630?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17921350#comment-17921350
]
Jason Gerlowski commented on SOLR-17630:
bq. SolrClientCache is largely an unnecessary mechanism outside of
streaming-expressions. We don't need to cache SolrClient instances by a URL key
Curious for more details there. What makes it necessary in
streaming-expressions, but unnecessary elsewhere? (Or is it "unnecessary"
everywhere, but prohibitively difficult to untangle from certain places like
streaming-expressions?).
To Eric's point - allowing usage only within a particular module seems like a
hard line to hold. Most folks will just see it as an IDE autocomplete option
when they type "coreContainer.get", and blithely go on their way. That's what
I blame for my mistaken usage the other day.
> Add CloudSolrClient instance for a Solr node
>
>
> Key: SOLR-17630
> URL: https://issues.apache.org/jira/browse/SOLR-17630
> Project: Solr
> Issue Type: Improvement
> Components: SolrCloud
>Reporter: David Smiley
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> There ought to be a general CloudSolrClient instance for the Solr node,
> without each potential user of such needing to create one. The closest
> substitute at the moment is
> {{cc.getSolrClientCache().getCloudSolrClient(cc.getZkController().getZkServerAddress())}}
> which is too verbose, not as discoverable, and it's debatable if
> SolrClientCache should be it's home.
> A scalability/simplicity advantage of a shared one instead of newly
> constructed one is that the existing ZkClientClusterStateProvider (same node
> ZkStateReader instance) can be used, thus improving scalability and
> simplifying interpretation of logs (as all logs from ZkStateReader on a node
> can be assumed to then be from the same instance). SolrClientCache creates
> new ones.
--
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] SolrParams.equals implementation [solr]
renatoh commented on code in PR #3141:
URL: https://github.com/apache/solr/pull/3141#discussion_r1930665571
##
solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java:
##
@@ -526,4 +527,36 @@ public String toString() {
}
return sb.toString();
}
+
+ /**
+ * A SolrParams is equal to another if they have the same keys and values.
The order of keys does
+ * not matter.
+ */
+ @Override
+ public boolean equals(Object obj) {
+if (obj == this) return true;
+if (!(obj instanceof SolrParams b)) return false;
+
+// iterating this params, see if other has the same values for each key
+int count = 0;
+for (Entry thisEntry : this) {
+ String name = thisEntry.getKey();
+ if (!Arrays.equals(thisEntry.getValue(), b.getParams(name))) return
false;
+ count++;
+}
+// does other params have the same number of keys? It might have more but
not less.
+Iterator bNames = b.getParameterNamesIterator();
+while (bNames.hasNext()) {
Review Comment:
It looks to me, like this equals method is not symmetric:.equals(y) must
return the same result as y.equals(x).
--
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-14414: Introduce new UI (SIP-7) [solr]
malliaridis commented on code in PR #2605:
URL: https://github.com/apache/solr/pull/2605#discussion_r1930746093
##
solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt:
##
@@ -0,0 +1,257 @@
+/*
+ * 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.composeui.ui.icons
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.material.icons.materialPath
+import androidx.compose.material3.Icon
+import androidx.compose.material3.MaterialTheme
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.vector.ImageVector
+import androidx.compose.ui.graphics.vector.rememberVectorPainter
+import androidx.compose.ui.unit.dp
+import org.apache.solr.compose_ui.generated.resources.Res
+import org.apache.solr.compose_ui.generated.resources.cd_solr_logo
+import org.jetbrains.compose.resources.stringResource
+
+/**
+ * Colorized Solr logo.
+ *
+ * @param modifier Modifier that is applied to the wrapper of the logo.
+ */
+@Composable
+fun SolrLogo(
+modifier: Modifier = Modifier,
+) {
+val solrPainter = rememberVectorPainter(
+ImageVector.Builder(
+name = "Logos.SolrLogo",
+defaultWidth = 128.dp,
+defaultHeight = 64.dp,
+viewportWidth = 128f,
+viewportHeight = 64f,
+autoMirror = false,
+).apply {
+materialPath {
+moveTo(25.7013f, 44.178f)
Review Comment:
I have simplified this with actual SVGs, one for the light theme (classic)
and one for the dark theme.
The only limitation here is that the SVGs do not work on Android, so we have
to migrate when we plan to support mobile devices. 😂
--
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] Remove broken and unused ZkStateReader.getReplicaProps() [solr]
psalagnac merged PR #3059: URL: https://github.com/apache/solr/pull/3059 -- 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-16903: Migrate off java.io.File to java.nio.file.Path from core files [solr]
mlbiscoc commented on code in PR #2924:
URL: https://github.com/apache/solr/pull/2924#discussion_r1929246536
##
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##
@@ -373,12 +372,12 @@ public InputStream openResource(String resource) throws
IOException {
// Delegate to the class loader (looking into $INSTANCE_DIR/lib jars).
// We need a ClassLoader-compatible (forward-slashes) path here!
-InputStream is =
classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'));
+InputStream is = classLoader.getResourceAsStream(resource);
// This is a hack just for tests (it is not done in ZKResourceLoader)!
// TODO can we nuke this?
if (is == null && System.getProperty("jetty.testMode") != null) {
- is = classLoader.getResourceAsStream(("conf/" +
resource).replace(File.separatorChar, '/'));
+ is =
classLoader.getResourceAsStream(Path.of("conf").resolve(resource).toString());
Review Comment:
I relooked at `File.separatorChar` throughout this PR and moved them to
`FileSystems.getDefault().getSeparator()` where it might make sense instead of
moving it to PATH to avoid as little breaking as possible.
##
solr/core/src/test/org/apache/solr/core/ResourceLoaderTest.java:
##
@@ -81,10 +81,9 @@ public void testEscapeInstanceDir() throws Exception {
// UNC paths
assertTrue(
assertThrows(
- SolrResourceNotFoundException.class,
- () -> loader.openResource("192.168.10.10\\foo").close())
+ SolrException.class, () ->
loader.openResource("192.168.10.10\\foo").close())
Review Comment:
I did so because of a change from this [comment I
made](https://github.com/apache/solr/pull/2924#discussion_r1918615357) from
switching to assertNotUnc.
##
solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java:
##
@@ -203,9 +202,8 @@ public OutputStream openOutputStream(String
storedResourceId) throws IOException
@Override
public boolean delete(String storedResourceId) throws IOException {
- // TODO SOLR-8282 move to PATH
- File storedFile = new File(storageDir, storedResourceId);
- return deleteIfFile(storedFile.toPath());
+ Path storedFile = Path.of(storageDir, storedResourceId);
Review Comment:
So we need to move `storageDir` to Path entirely in this class but in
another JIRA? I can skip this, but we will need to keep `new File(storageDir,
storedResourceId);`
##
solr/core/src/java/org/apache/solr/util/VersionedFile.java:
##
@@ -41,44 +41,48 @@ public class VersionedFile {
*/
public static InputStream getLatestFile(String dirName, String fileName)
throws FileNotFoundException {
-// TODO SOLR-8282 move to PATH
-Collection oldFiles = null;
+Collection oldFiles = null;
final String prefix = fileName + '.';
-File f = new File(dirName, fileName);
+Path f = Path.of(dirName, fileName);
InputStream is = null;
// there can be a race between checking for a file and opening it...
// the user may have just put a new version in and deleted an old version.
// try multiple times in a row.
for (int retry = 0; retry < 10 && is == null; retry++) {
try {
-if (!f.exists()) {
- File dir = new File(dirName);
- String[] names =
- dir.list(
- new FilenameFilter() {
-@Override
-public boolean accept(File dir, String name) {
- return name.startsWith(prefix);
-}
- });
- Arrays.sort(names);
- f = new File(dir, names[names.length - 1]);
+if (!Files.exists(f)) {
+ Path dir = Path.of(dirName);
+ List fileList;
+
+ try (Stream files = Files.list(dir)) {
+fileList =
+files
+.filter((file) ->
file.getFileName().toString().startsWith(prefix))
Review Comment:
Reading java docs I see `The elements of the stream are Path objects that
are obtained as if by resolving the name of the directory entry against dir.`
So I believe that means that the path could be relative but the prefix will
contain the directory is resolved against. So if we are checking the file
starts with a specific prefix, we need `getFileName()`
##
solr/core/src/java/org/apache/solr/core/SolrPaths.java:
##
@@ -94,7 +94,7 @@ public static void assertNoPathTraversal(Path pathToAssert) {
/** Asserts that a path is not a Windows UNC path */
public static void assertNotUnc(Path pathToAssert) {
-if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("")) {
+if (OS.isFamilyWindows() || pathToAssert.toString().startsWith("")) {
Review Comment:
I mentioned it in this comment
[here](https://github.com/apache/solr/pull/2924#discussion_r191861535
Re: [PR] SolrParams.equals implementation [solr]
dsmiley commented on code in PR #3141:
URL: https://github.com/apache/solr/pull/3141#discussion_r1930815270
##
solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java:
##
@@ -526,4 +527,36 @@ public String toString() {
}
return sb.toString();
}
+
+ /**
+ * A SolrParams is equal to another if they have the same keys and values.
The order of keys does
+ * not matter.
+ */
+ @Override
+ public boolean equals(Object obj) {
+if (obj == this) return true;
+if (!(obj instanceof SolrParams b)) return false;
+
+// iterating this params, see if other has the same values for each key
+int count = 0;
+for (Entry thisEntry : this) {
+ String name = thisEntry.getKey();
+ if (!Arrays.equals(thisEntry.getValue(), b.getParams(name))) return
false;
+ count++;
+}
+// does other params have the same number of keys? It might have more but
not less.
+Iterator bNames = b.getParameterNamesIterator();
+while (bNames.hasNext()) {
Review Comment:
Can you suggest improvements to `testTrivialEquals` to prove your theory?
--
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] remove deprecated (DocValues,Norms)FieldExistsQuery use [solr]
HoustonPutman commented on PR #2632: URL: https://github.com/apache/solr/pull/2632#issuecomment-2616441824 > Suggest `main` branch only, since now changes for https://issues.apache.org/jira/browse/SOLR-14199 are also included, WDYT? So that Jira isn't actually done here. Inside Solr is removing norms on the field, but it's not giving users an error when it's defined. So it should be back-compat... Let's see that the tests pass with this though. -- 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] remove deprecated (DocValues,Norms)FieldExistsQuery use [solr]
cpoerschke commented on code in PR #2632:
URL: https://github.com/apache/solr/pull/2632#discussion_r1930845447
##
solr/core/src/java/org/apache/solr/schema/DenseVectorField.java:
##
@@ -394,7 +394,7 @@ public Query getFieldQuery(QParser parser, SchemaField
field, String externalVal
/** Not Supported */
Review Comment:
Wondering if we might wanna
```
@Override
protected boolean treatUnboundedRangeAsExistence(SchemaField field) {
return false;
}
```
to retain the behaviour of `getRangeQuery` throwing an exception.
--
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] remove deprecated (DocValues,Norms)FieldExistsQuery use [solr]
cpoerschke commented on code in PR #2632: URL: https://github.com/apache/solr/pull/2632#discussion_r1930872830 ## solr/core/src/java/org/apache/solr/schema/DenseVectorField.java: ## @@ -394,7 +394,7 @@ public Query getFieldQuery(QParser parser, SchemaField field, String externalVal /** Not Supported */ Review Comment: Answering own question: it already returns `false` via this implementation in the class hierarchy: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/schema/NumericFieldType.java#L360 -- 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] remove deprecated (DocValues,Norms)FieldExistsQuery use [solr]
cpoerschke commented on PR #2632: URL: https://github.com/apache/solr/pull/2632#issuecomment-2616389577 > for `main` and `branch_9x` only, no JIRA or solr/CHANGES.txt entry needed in my opinion. Suggest `main` branch only, since now changes for https://issues.apache.org/jira/browse/SOLR-14199 are also included, WDYT? -- 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-17630) Add CloudSolrClient instance for a Solr node
[
https://issues.apache.org/jira/browse/SOLR-17630?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17921462#comment-17921462
]
David Smiley commented on SOLR-17630:
-
SolrClientCache could be removed entirely in Solr 10 if we invest effort
accordingly. I'm just trying to clarify a path forward with incremental
progress without forcing us having to change everything everywhere at once
(i.e. remove SCC in one go). Perhaps we make CoreContainer.getSolrClientCache
deprecated. I'm thinking we should use deprecation liberally in our APIs, even
before we're ready to remove the thing.
> Add CloudSolrClient instance for a Solr node
>
>
> Key: SOLR-17630
> URL: https://issues.apache.org/jira/browse/SOLR-17630
> Project: Solr
> Issue Type: Improvement
> Components: SolrCloud
>Reporter: David Smiley
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> There ought to be a general CloudSolrClient instance for the Solr node,
> without each potential user of such needing to create one. The closest
> substitute at the moment is
> {{cc.getSolrClientCache().getCloudSolrClient(cc.getZkController().getZkServerAddress())}}
> which is too verbose, not as discoverable, and it's debatable if
> SolrClientCache should be it's home.
> A scalability/simplicity advantage of a shared one instead of newly
> constructed one is that the existing ZkClientClusterStateProvider (same node
> ZkStateReader instance) can be used, thus improving scalability and
> simplifying interpretation of logs (as all logs from ZkStateReader on a node
> can be assumed to then be from the same instance). SolrClientCache creates
> new ones.
--
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] SolrParams.equals implementation [solr]
renatoh commented on code in PR #3141:
URL: https://github.com/apache/solr/pull/3141#discussion_r1931638863
##
solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java:
##
@@ -526,4 +527,36 @@ public String toString() {
}
return sb.toString();
}
+
+ /**
+ * A SolrParams is equal to another if they have the same keys and values.
The order of keys does
+ * not matter.
+ */
+ @Override
+ public boolean equals(Object obj) {
+if (obj == this) return true;
+if (!(obj instanceof SolrParams b)) return false;
+
+// iterating this params, see if other has the same values for each key
+int count = 0;
+for (Entry thisEntry : this) {
+ String name = thisEntry.getKey();
+ if (!Arrays.equals(thisEntry.getValue(), b.getParams(name))) return
false;
+ count++;
+}
+// does other params have the same number of keys? It might have more but
not less.
+Iterator bNames = b.getParameterNamesIterator();
+while (bNames.hasNext()) {
Review Comment:
I could not find a test case where x.equals(y) is not y.equals(y)
I missed the fact, that params with the same key are combined into the same
Array, my bad!
--
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] Jetty12 + EE10 [solr]
dsmiley commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r1931628768
##
solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java:
##
@@ -93,40 +114,66 @@ private void assertCoreChosen(int numCores, TestRequest
testRequest) throws Unav
assertEquals(numCores, coreNames.size());
}
- private static class TestResponse extends Response {
+ public static class TestRequest implements HttpServletRequest {
Review Comment:
I hate mocks but as I look at this, mocking seems perfect so you don't have
to implement everything. Or maybe there's a base class as there was before
that you can extend. (Same feedback for another source file you did this in)
##
solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java:
##
@@ -106,7 +106,7 @@ public static void setupCluster() throws Exception {
((Metered)
metricRegistry
.getMetrics()
-
.get("org.eclipse.jetty.servlet.ServletContextHandler.2xx-responses"))
+
.get("org.eclipse.jetty.server.Handler$Wrapper.2xx-responses"))
Review Comment:
This metrics name seems poor. I wonder if it's due to us/Solr (thus we have
easy control) or is this name chosen by Jetty / DropWizard.
##
solr/modules/s3-repository/build.gradle:
##
@@ -20,7 +20,9 @@ apply plugin: 'java-library'
description = 'S3 Repository'
dependencies {
- api project(':solr:core')
+ api(project(':solr:core')) {
+exclude group: 'org.ow2.asm', module: '*'
Review Comment:
them solr:core shouldn't be publishing that dependency in the first place.
At least see if that can be done.
##
solr/cross-dc-manager/src/java/org/apache/solr/crossdc/manager/consumer/Consumer.java:
##
@@ -21,8 +21,8 @@
import static
org.apache.solr.crossdc.common.KafkaCrossDcConf.ZK_CONNECT_STRING;
import com.codahale.metrics.SharedMetricRegistries;
-import com.codahale.metrics.servlets.MetricsServlet;
-import com.codahale.metrics.servlets.ThreadDumpServlet;
+import io.dropwizard.metrics.servlets.MetricsServlet;
Review Comment:
it's suspicious to see we import com.codahale & io.dropwizard. Maybe okay
but I wasn't expecting.
##
solr/server/etc/jetty.xml:
##
@@ -214,14 +212,16 @@
+
