Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]
epugh commented on PR #3047: URL: https://github.com/apache/solr/pull/3047#issuecomment-2614614636 I enjoyed catching up on the comments on 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
[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=17921218#comment-17921218
]
Eric Pugh commented on SOLR-17630:
--
Would giving it a more specific name like StreamingExpressionsSolrClientCache
help? Can it be in the Streaming Expressions jar so it isn't used elsewhere?
> 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] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1929924679
##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -282,7 +282,6 @@ private void commitOnLeader(String leaderBaseUrl, String
coreName)
throws SolrServerException, IOException {
try (SolrClient client = recoverySolrClientBuilder(leaderBaseUrl,
coreName).build()) {
UpdateRequest ureq = new UpdateRequest();
- ureq.setParams(new ModifiableSolrParams());
Review Comment:
This was clearly a hack to work around it's weird mutability situation. Now
not needed here and elsewhere.
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java:
##
@@ -20,13 +20,15 @@
import
org.apache.solr.client.solrj.request.RequestWriter.StringPayloadContentWriter;
import org.apache.solr.client.solrj.response.UpdateResponse;
import org.apache.solr.client.solrj.util.ClientUtils;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
/**
* Send arbitrary XML to a request handler
*
* @since solr 1.3
*/
+@Deprecated
Review Comment:
There's a lot I want to deprecate and remove in Solr 10; this is one. The
annotation snuck in; arguably for another PR. On the other hand, Solr isn't
using or testing this class, so let's get on with removing!
##
solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java:
##
@@ -625,11 +625,8 @@ protected long addDocAndGetVersion(Object... fields)
throws Exception {
SolrInputDocument doc = new SolrInputDocument();
addFields(doc, fields);
-ModifiableSolrParams params = new ModifiableSolrParams();
-params.add("versions", "true");
-
UpdateRequest ureq = new UpdateRequest();
-ureq.setParams(params);
+ureq.getParams().set("versions", true);
Review Comment:
With a clear mutability story, the requirement here is now easy with this
one-liner. (same elsewhere)
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -380,7 +380,7 @@ public InputStreamResponseListener getResponseListener() {
public OutStream initOutStream(String baseUrl, UpdateRequest updateRequest,
String collection)
throws IOException {
String contentType = requestWriter.getUpdateContentType();
-final ModifiableSolrParams origParams = new
ModifiableSolrParams(updateRequest.getParams());
+final SolrParams origParams = updateRequest.getParams();
Review Comment:
My suspicion is that `new ModifiableSolrParams` is here to implicilty handle
null (as it handles that in its constructor). We have no need to modify the
result, I confirm.
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/AbstractUpdateRequest.java:
##
@@ -21,10 +21,9 @@
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.UpdateParams;
-/** */
public abstract class AbstractUpdateRequest extends
CollectionRequiringSolrRequest
implements IsUpdateRequest {
- protected ModifiableSolrParams params;
+ protected ModifiableSolrParams params = new ModifiableSolrParams(); // TODO
make final
Review Comment:
Up for debate on eventually final. That means we lose the setter, which is
called in a ton of places, in lieu of calling `getParams().add(params)`. I
actually went down that path and shelved the work. I was motivated for clarity
on the getParams value never being potentially shared with something else.
##
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroredSolrRequest.java:
##
@@ -72,7 +71,7 @@ public SolrParams getParams() {
return params;
}
-public void setParams(ModifiableSolrParams params) {
+public void setParams(SolrParams params) {
Review Comment:
getter & setter should have same type
##
solr/core/src/test/org/apache/solr/update/processor/TemplateUpdateProcessorTest.java:
##
@@ -69,14 +71,11 @@ public void testSimple() throws Exception {
SolrInputDocument solrDoc = new SolrInputDocument();
solrDoc.addField("id", "1");
-params =
-new ModifiableSolrParams()
-.add("processor", "template")
-.add("commit", "true")
-.add("template.field", "x_s:key_{id}");
-params.add("commit", "true");
UpdateRequest add = new UpdateRequest().add(solrDoc);
-add.setParams(params);
+add.getParams()
+.add("processor", "template")
+.add("template.field", "x_s:key_{id}")
+.add("commit", "true");
Review Comment:
so nice IMO
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java:
##
@@ -16,36 +16,39 @@
*/
package org.apache.solr.client.solrj.request;
+import java.util.Objects;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.
[PR] Add security notices for two recent CVEs [solr-site]
gerlowskija opened a new pull request, #141: URL: https://github.com/apache/solr-site/pull/141 - CVE-2024-52012 - CVE-2025-24814 -- 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 security notices for two recent CVEs [solr-site]
gerlowskija merged PR #141: URL: https://github.com/apache/solr-site/pull/141 -- 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-17351: Decompose filestore "get file" API [solr]
gerlowskija commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1929150292
##
solr/api/src/java/org/apache/solr/client/api/endpoint/ClusterFileStoreApis.java:
##
@@ -70,4 +97,23 @@ SolrJerseyResponse deleteFile(
"Indicates whether the deletion should only be done on the
receiving node. For internal use only")
@QueryParam("localDelete")
Boolean localDelete);
+
+ @POST
+ @Operation(
+ summary =
+ "Pushes a file to other nodes, or pulls a file from other nodes in
the Solr cluster.",
Review Comment:
(This ends up being moot since I take your "split up the endpoint"
suggestion on L116)
##
solr/core/src/java/org/apache/solr/filestore/ClusterFileStore.java:
##
@@ -141,6 +151,103 @@ public UploadToFileStoreResponse uploadFile(
return response;
}
+ @Override
+ @PermissionName(PermissionNameProvider.Name.FILESTORE_READ_PERM)
+ public SolrJerseyResponse getFile(String path) {
+final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
+attachFileToResponse(path, fileStore, req, rsp);
+return response;
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.FILESTORE_READ_PERM)
+ public FileStoreDirectoryListingResponse getMetadata(String path) {
+if (path == null) {
+ path = "";
+}
+FileStore.FileType type = fileStore.getType(path, false);
+return getMetadata(type, path, fileStore);
+ }
+
+ public static void attachFileToResponse(
+ String path, FileStore fileStore, SolrQueryRequest req,
SolrQueryResponse rsp) {
+ModifiableSolrParams solrParams = new ModifiableSolrParams();
+solrParams.add(CommonParams.WT, FILE_STREAM);
+req.setParams(SolrParams.wrapDefaults(solrParams, req.getParams()));
+rsp.add(
+CONTENT,
+(SolrCore.RawWriter)
+os ->
+fileStore.get(
+path,
+it -> {
+ try {
+InputStream inputStream = it.getInputStream();
+if (inputStream != null) {
Review Comment:
I'm not sure honestly. I could imagine it being null in some testing
scenarios (a mock-based test, or something that runs with less than the full
cluster like SolrCloudTestCase-based tests establishes)
But these lines aren't new to this PR, they're just moved from
[NodeFileStore](https://github.com/apache/solr/pull/3047/files#diff-659576ead9b46b2a7b9719a94417add657c7dc8e01b39cc416a3a6cfb6922ffcL148),
so I'll probably avoid trying to tweak them too much here...
##
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##
@@ -507,7 +503,8 @@ public void delete(String path) {
final var solrParams = new ModifiableSolrParams();
solrParams.add("localDelete", "true");
-final var solrRequest = new GenericSolrRequest(DELETE, "/cluster/files" +
path, solrParams);
+final var solrRequest =
+new GenericSolrRequest(DELETE, "/cluster/filestore/files" + path,
solrParams);
Review Comment:
Huh - I wonder why I didn't change this at the time? Anyway, fixed.
##
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##
@@ -189,26 +187,32 @@ private boolean fetchFileFromNodeAndPersist(String
fromNode) {
var solrClient = coreContainer.getDefaultHttpSolrClient();
try {
-GenericSolrRequest request = new GenericSolrRequest(GET, "/node/files"
+ getMetaPath());
-request.setResponseParser(new InputStreamResponseParser(null));
-var response = solrClient.requestWithBaseUrl(baseUrl,
request::process).getResponse();
-is = (InputStream) response.get("stream");
+final var metadataRequest = new FileStoreApi.GetFile(getMetaPath());
+final var client =
coreContainer.getSolrClientCache().getHttpSolrClient(baseUrl);
+final var response = metadataRequest.process(client);
metadata =
-Utils.newBytesConsumer((int) MAX_PKG_SIZE).accept((InputStream)
response.get("stream"));
+Utils.newBytesConsumer((int) MAX_PKG_SIZE)
+.accept(response.getResponseStreamIfSuccessful());
m = (Map) Utils.fromJSON(metadata.array(),
metadata.arrayOffset(), metadata.limit());
- } catch (SolrServerException | IOException e) {
+ } catch (Exception e) {
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error
fetching metadata", e);
} finally {
org.apache.solr.common.util.IOUtils.closeQuietly(is);
}
+ ByteBuffer filedata = null;
+ try {
+final var fileRequest = new FileStoreApi.GetFile(path);
+final var client =
coreContainer.getSolrClientCache().getHttpSolrClient(baseUrl);
Review Comment:
Reverted to `requestWithBaseUrl`.
I'm a little confused about SolrClientCache going away, or only being
suitable f
[jira] [Commented] (SOLR-17519) CloudSolrClient with HTTP ClusterState can forget live nodes and then fail
[
https://issues.apache.org/jira/browse/SOLR-17519?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17921263#comment-17921263
]
ASF subversion and git services commented on SOLR-17519:
Commit 916ae15387e368ac546a8174955e4d33fca37239 in solr's branch
refs/heads/branch_9x from Matthew Biscocho
[ https://gitbox.apache.org/repos/asf?p=solr.git;h=916ae15387e ]
SOLR-17519: CloudSolrClient's HTTP ClusterStateProvider now uses configured
URLs as backup (#2935)
SolrJ CloudSolrClient configured with Solr URLs can fail to request cluster
state if its current live nodes list are all unavailable. The
HttpClusterStateProvider now retains the initial configured list of passed URLs
as backup.
Utils.getBaseUrlForNodeName moved to URLUtil.
MiniSolrCloudCluster.startJettySolrRunner is overloaded now to choose to reuse
the port.
(cherry picked from commit b1fe883991e59e8bed449655e9db1d5cbc77f1de)
> CloudSolrClient with HTTP ClusterState can forget live nodes and then fail
> --
>
> Key: SOLR-17519
> URL: https://issues.apache.org/jira/browse/SOLR-17519
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Components: SolrCloud, SolrJ
>Reporter: David Smiley
>Priority: Major
> Labels: newdev, pull-request-available
> Time Spent: 6h 10m
> Remaining Estimate: 0h
>
> When using CloudSolrClient with HTTP URLs to Solr for the cluster state:
> If all live nodes disappear temporarily (hard cluster restart?), the client
> can permanently fail to talk to the cluster, and thus would need to be
> restarted to recover.
> Credit [~ilan] on the dev list:
> {quote}The current implementation removes non live nodes from the set of
> nodes to connect to. Getting the live nodes requires connecting to a specific
> node in the cluster that is therefore live when that happens. Worst case, if
> there is a single node up in the cluster, the client ends with a single node
> in its connection candidates list. For the issue to manifest, that Solr node
> then has to go down. Subsequently, even if other nodes are up, the client
> only has the address of a down node and can't connect.
> The fix is not a big deal. Nodes initially passed as configuration to the
> client should never be removed from the set of candidate nodes to connect to,
> even if they are not live. Other live nodes could be added to that set (and
> removed from it if we so desire when they are no longer live) to increase
> resiliency in case the cluster does have live nodes but all initially
> configured nodes are not live. The design issue is treating the configured
> set of nodes to connect to and the set of live nodes as one thing.
> {quote}
> See org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider
--
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-17321) Bump minimum required Java version to 21
[ https://issues.apache.org/jira/browse/SOLR-17321?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] David Smiley updated SOLR-17321: Fix Version/s: main (10.0) > Bump minimum required Java version to 21 > > > Key: SOLR-17321 > URL: https://issues.apache.org/jira/browse/SOLR-17321 > Project: Solr > Issue Type: Task >Reporter: Sanjay Dutt >Priority: Major > Labels: pull-request-available > Fix For: main (10.0) > > Time Spent: 11h 50m > Remaining Estimate: 0h > > We are upgrading the minimum Java version for Solr main branch to 21. > However, at the same, It has been suggested to be not so aggressive with > SolrJ (and thus solr-api, a dependency) Java version – setting it to 17. -- 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] [Created] (SOLR-17635) javabin should deserialize maps as SimpleOrderedMap
David Smiley created SOLR-17635: --- Summary: javabin should deserialize maps as SimpleOrderedMap Key: SOLR-17635 URL: https://issues.apache.org/jira/browse/SOLR-17635 Project: Solr Issue Type: Improvement Reporter: David Smiley Once SimpleOrderedMap actually implements Map (SOLR-17623), Solr's "javabin" format should deserialize all maps as a SimpleOrderedMap. This will make it easier to transition away from NamedList/SimpleOrderedMap in responses (such as to a Map or MapWriter) without worry of impacting javabin clients that still expect a NamedList. It may also increase deserialization performance & lower memory at the expense of any former Maps (thus were deserialized as LinkedHashMap, O(1) lookup) becoming O(N) lookup. -- 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-17635) javabin should deserialize maps as SimpleOrderedMap
[ https://issues.apache.org/jira/browse/SOLR-17635?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17921264#comment-17921264 ] David Smiley commented on SOLR-17635: - I guess this could go to 9.9; why not? Or a system-property toggle in 9.9 if there is some concern. > javabin should deserialize maps as SimpleOrderedMap > --- > > Key: SOLR-17635 > URL: https://issues.apache.org/jira/browse/SOLR-17635 > Project: Solr > Issue Type: Improvement >Reporter: David Smiley >Priority: Major > > Once SimpleOrderedMap actually implements Map (SOLR-17623), Solr's "javabin" > format should deserialize all maps as a SimpleOrderedMap. This will make it > easier to transition away from NamedList/SimpleOrderedMap in responses (such > as to a Map or MapWriter) without worry of impacting javabin clients that > still expect a NamedList. > It may also increase deserialization performance & lower memory at the > expense of any former Maps (thus were deserialized as LinkedHashMap, O(1) > lookup) becoming O(N) lookup. -- 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
