Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]

[via GitHub]

gerlowskija merged PR #3047:
URL: https://github.com/apache/solr/pull/3047


-- 
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]

[via GitHub]

gerlowskija commented on PR #3047:
URL: https://github.com/apache/solr/pull/3047#issuecomment-2637426498

   Alright - I think I've addressed the feedback so far?  If I've missed 
anything, let me know.  I've brought it up to date with 'main' and will aim to 
merge in the next few days pending any objections?


-- 
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]

[via GitHub]

gerlowskija commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1943253094


##
solr/core/src/java/org/apache/solr/filestore/NodeFileStore.java:
##
@@ -76,135 +66,54 @@ public SolrJerseyResponse getFile(String path, Boolean 
sync, String getFrom, Boo
 final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
 
 if (Boolean.TRUE.equals(sync)) {
-  try {
-fileStore.syncToAllNodes(path);
-return response;
-  } catch (IOException e) {
-throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Error 
getting file ", e);
-  }
+  ClusterFileStore.syncToAllNodes(fileStore, path);

Review Comment:
   Alright, the most recent iteration removes NodeFileStore/NodeFileStoreApi.  
There's still some room for confusion between ClusterFileStoreApi, 
ClusterFileStore, and DistribFileStore, but overall this PR leaves things 
better than it found it in that regard 👍 



-- 
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]

[via GitHub]

gerlowskija commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1941803992


##
solr/core/src/java/org/apache/solr/filestore/NodeFileStore.java:
##
@@ -76,135 +66,54 @@ public SolrJerseyResponse getFile(String path, Boolean 
sync, String getFrom, Boo
 final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
 
 if (Boolean.TRUE.equals(sync)) {
-  try {
-fileStore.syncToAllNodes(path);
-return response;
-  } catch (IOException e) {
-throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Error 
getting file ", e);
-  }
+  ClusterFileStore.syncToAllNodes(fileStore, path);
+  return response;
 }
 
 if (path == null) {
   path = "";
 }
 final var pathCopy = path;
 if (getFrom != null) {
-  coreContainer
-  .getUpdateShardHandler()
-  .getUpdateExecutor()
-  .submit(
-  () -> {
-log.debug("Downloading file {}", pathCopy);
-try {
-  fileStore.fetch(pathCopy, getFrom);
-} catch (Exception e) {
-  log.error("Failed to download file: {}", pathCopy, e);
-}
-log.info("downloaded file: {}", pathCopy);
-  });
+  ClusterFileStore.pullFileFromNode(coreContainer, fileStore, pathCopy, 
getFrom);
   return response;
 }
 
 FileStore.FileType type = fileStore.getType(path, false);
-if (type == FileStore.FileType.NOFILE) {
-  final var fileMissingResponse =
-  instantiateJerseyResponse(FileStoreDirectoryListingResponse.class);
-  fileMissingResponse.files = Collections.singletonMap(path, null);
-  return fileMissingResponse;
-}
-if (type == FileStore.FileType.DIRECTORY) {
-  final var directoryListingResponse =
-  instantiateJerseyResponse(FileStoreDirectoryListingResponse.class);
-  final var directoryContents =
-  fileStore.list(path, null).stream()
-  .map(details -> convertToResponse(details))
-  .collect(Collectors.toList());
-  directoryListingResponse.files = Collections.singletonMap(path, 
directoryContents);
-  return directoryListingResponse;
+if (type == FileStore.FileType.NOFILE
+|| type == FileStore.FileType.DIRECTORY
+|| (type == FileStore.FileType.FILE && Boolean.TRUE.equals(meta))) {
+  return ClusterFileStore.getMetadata(type, path, fileStore);
 }
-if (Boolean.TRUE.equals(meta)) {
-  if (type == FileStore.FileType.FILE) {
-int idx = path.lastIndexOf('/');
-String fileName = path.substring(idx + 1);
-String parentPath = path.substring(0, path.lastIndexOf('/'));
-List l = fileStore.list(parentPath, s -> 
s.equals(fileName));
 
-final var fileMetaResponse =
-instantiateJerseyResponse(FileStoreDirectoryListingResponse.class);
-fileMetaResponse.files =
-Collections.singletonMap(path, l.isEmpty() ? null : 
convertToResponse(l.get(0)));
-return fileMetaResponse;
-  }
-} else { // User wants to get the "raw" file
-  // TODO Should we be trying to json-ify otherwise "raw" files in this 
way?  It seems like a
-  // pretty sketchy idea, esp. for code with very little test coverage.  
Consider removing
-  if ("json".equals(req.getParams().get(CommonParams.WT))) {
-final var jsonResponse = 
instantiateJerseyResponse(FileStoreJsonFileResponse.class);
-try {
-  fileStore.get(
-  pathCopy,
-  it -> {
-try {
-  InputStream inputStream = it.getInputStream();
-  if (inputStream != null) {
-jsonResponse.response = new 
String(inputStream.readAllBytes(), UTF_8);
-  }
-} catch (IOException e) {
-  throw new SolrException(
-  SolrException.ErrorCode.SERVER_ERROR, "Error reading 
file " + pathCopy);
+// User wants to get the "raw" file
+// TODO Should we be trying to json-ify otherwise "raw" files in this way? 
 It seems like a

Review Comment:
   There's no discussion of it either in the PR or the JIRA that introduced it 
(unless I'm missing something?).  My guess is that it was an attempt at 
convenience?
   
   In any case; I think this entire file can now go.  See my comment 
[above](https://github.com/apache/solr/pull/3047/files#r1929771967) for more 
details.



-- 
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.apa

Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]

[via GitHub]

gerlowskija commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1940306565


##
solr/solr-ref-guide/modules/configuration-guide/pages/package-manager-internals.adoc:
##
@@ -115,7 +115,7 @@ openssl dgst -sha1 -sign my_key.pem runtimelibs.jar | 
openssl enc -base64 | sed
 +
 [source, bash]
 
-curl --data-binary @runtimelibs.jar -X PUT  
http://localhost:8983/api/cluster/files/mypkg/1.0/myplugins.jar?sig=
+curl --data-binary @runtimelibs.jar -X PUT  
http://localhost:8983/api/cluster/filestore/files/mypkg/1.0/myplugins.jar?sig=

Review Comment:
   (See other comments - "files" is our way to distinguish between "metadata" 
and "commands", which are both under the "/filestore" path.)



-- 
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]

[via GitHub]

gerlowskija commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1940305756


##
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##
@@ -173,7 +173,7 @@ public void uninstall(String packageName, String version)
 String.format(Locale.ROOT, "/package/%s/%s/%s", packageName, version, 
"manifest.json"));
 for (String filePath : filesToDelete) {
   DistribFileStore.deleteZKFileEntry(zkClient, filePath);
-  String path = "/api/cluster/files" + filePath;
+  String path = "/api/cluster/filestore/files" + filePath;

Review Comment:
   > Can a filestore have anything under than files?
   
   It can: "metadata" and "commands" are both under the "filestore" path as 
siblings to "files".



-- 
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]

[via GitHub]

gerlowskija commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1940305043


##
solr/core/src/java/org/apache/solr/filestore/NodeFileStore.java:
##
@@ -76,135 +66,54 @@ public SolrJerseyResponse getFile(String path, Boolean 
sync, String getFrom, Boo
 final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
 
 if (Boolean.TRUE.equals(sync)) {
-  try {
-fileStore.syncToAllNodes(path);
-return response;
-  } catch (IOException e) {
-throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Error 
getting file ", e);
-  }
+  ClusterFileStore.syncToAllNodes(fileStore, path);
+  return response;
 }
 
 if (path == null) {
   path = "";
 }
 final var pathCopy = path;
 if (getFrom != null) {
-  coreContainer
-  .getUpdateShardHandler()
-  .getUpdateExecutor()
-  .submit(
-  () -> {
-log.debug("Downloading file {}", pathCopy);
-try {
-  fileStore.fetch(pathCopy, getFrom);
-} catch (Exception e) {
-  log.error("Failed to download file: {}", pathCopy, e);
-}
-log.info("downloaded file: {}", pathCopy);
-  });
+  ClusterFileStore.pullFileFromNode(coreContainer, fileStore, pathCopy, 
getFrom);
   return response;
 }
 
 FileStore.FileType type = fileStore.getType(path, false);
-if (type == FileStore.FileType.NOFILE) {
-  final var fileMissingResponse =
-  instantiateJerseyResponse(FileStoreDirectoryListingResponse.class);
-  fileMissingResponse.files = Collections.singletonMap(path, null);
-  return fileMissingResponse;
-}
-if (type == FileStore.FileType.DIRECTORY) {
-  final var directoryListingResponse =
-  instantiateJerseyResponse(FileStoreDirectoryListingResponse.class);
-  final var directoryContents =
-  fileStore.list(path, null).stream()
-  .map(details -> convertToResponse(details))
-  .collect(Collectors.toList());
-  directoryListingResponse.files = Collections.singletonMap(path, 
directoryContents);
-  return directoryListingResponse;
+if (type == FileStore.FileType.NOFILE
+|| type == FileStore.FileType.DIRECTORY
+|| (type == FileStore.FileType.FILE && Boolean.TRUE.equals(meta))) {
+  return ClusterFileStore.getMetadata(type, path, fileStore);
 }
-if (Boolean.TRUE.equals(meta)) {
-  if (type == FileStore.FileType.FILE) {
-int idx = path.lastIndexOf('/');
-String fileName = path.substring(idx + 1);
-String parentPath = path.substring(0, path.lastIndexOf('/'));
-List l = fileStore.list(parentPath, s -> 
s.equals(fileName));
 
-final var fileMetaResponse =
-instantiateJerseyResponse(FileStoreDirectoryListingResponse.class);
-fileMetaResponse.files =
-Collections.singletonMap(path, l.isEmpty() ? null : 
convertToResponse(l.get(0)));
-return fileMetaResponse;
-  }
-} else { // User wants to get the "raw" file
-  // TODO Should we be trying to json-ify otherwise "raw" files in this 
way?  It seems like a
-  // pretty sketchy idea, esp. for code with very little test coverage.  
Consider removing
-  if ("json".equals(req.getParams().get(CommonParams.WT))) {
-final var jsonResponse = 
instantiateJerseyResponse(FileStoreJsonFileResponse.class);
-try {
-  fileStore.get(
-  pathCopy,
-  it -> {
-try {
-  InputStream inputStream = it.getInputStream();
-  if (inputStream != null) {
-jsonResponse.response = new 
String(inputStream.readAllBytes(), UTF_8);
-  }
-} catch (IOException e) {
-  throw new SolrException(
-  SolrException.ErrorCode.SERVER_ERROR, "Error reading 
file " + pathCopy);
+// User wants to get the "raw" file
+// TODO Should we be trying to json-ify otherwise "raw" files in this way? 
 It seems like a

Review Comment:
   Not that I know of.  I'm going to nuke it based on the discussion 
[here](https://github.com/apache/solr/pull/3047/files#r1929771967), and that 
might flush out something I don't understand.
   
   (We'll need to make sure we're only deprecating it in the 9.x backport, but 
that's not a big deal...)



-- 
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 

Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]

[via GitHub]

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

Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]

[via GitHub]

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

Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]

[via GitHub]

dsmiley commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1924452036


##
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:
   Please don't use the SolrClientCache for anything other than streaming 
expressions.  I'm stamping out such usages here: 
https://issues.apache.org/jira/browse/SOLR-17630
   I'm particularly surprised to see you removed a use of the new 
`requestWithBaseUrl`.



##
solr/core/src/java/org/apache/solr/filestore/ClusterFileStore.java:
##
@@ -194,6 +301,48 @@ public SolrJerseyResponse deleteFile(String filePath, 
Boolean localDelete) {
 return response;
   }
 
+  @Override
+  @PermissionName(PermissionNameProvider.Name.FILESTORE_WRITE_PERM)
+  public SolrJerseyResponse executeFileStoreCommand(String path, String 
getFrom, Boolean sync) {
+final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
+
+if (Boolean.TRUE.equals(sync)) {
+  syncToAllNodes(fileStore, path);
+} else if (getFrom != null) {
+  if (path == null) {
+path = "";
+  }
+  pullFileFromNode(coreContainer, fileStore, path, getFrom);
+}
+
+return response;
+  }
+
+  public static void syncToAllNodes(FileStore fileStore, String path) {
+try {
+  fileStore.syncToAllNodes(path);
+} catch (IOException e) {
+  throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Error 
getting file ", e);

Review Comment:
   We don't know if we can blame the user



-- 
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]

[via GitHub]

epugh commented on code in PR #3047:
URL: https://github.com/apache/solr/pull/3047#discussion_r1924229223


##
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:
   is the fact that one thing does a push or pull suggest a issue to fix?  
https://en.wikipedia.org/wiki/List_of_Doctor_Dolittle_characters#Pushmi-Pullyu



##
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.",
+  tags = {"file-store"})
+  @Path("/commands{path:.+}")
+  SolrJerseyResponse executeFileStoreCommand(
+  @Parameter(description = "Path to a file or directory within the 
filestore")
+  @PathParam("path")
+  String path,
+  @Parameter(description = "An optional Solr node name to fetch the file 
from")
+  @QueryParam("getFrom")
+  String getFrom,
+  @Parameter(
+  description =
+  "If true, triggers syncing for this file across all nodes in 
the filestore")

Review Comment:
   maybe it's own end point?  Or, do we somehow eliminate the need for this?



##
solr/core/src/java/org/apache/solr/filestore/ClusterFileStore.java:
##
@@ -194,6 +301,48 @@ public SolrJerseyResponse deleteFile(String filePath, 
Boolean localDelete) {
 return response;
   }
 
+  @Override
+  @PermissionName(PermissionNameProvider.Name.FILESTORE_WRITE_PERM)
+  public SolrJerseyResponse executeFileStoreCommand(String path, String 
getFrom, Boolean sync) {

Review Comment:
   aren't we trying to get rid of the "command" pattern in our new APIs?



##
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());

Review Comment:
   yay!



##
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##
@@ -364,11 +360,13 @@ private void distribute(FileInfo info) {
   for (String node : nodes) {
 String baseUrl =
 
coreContainer.getZkController().getZkStateReader().getBaseUrlV2ForNodeName(node);
+
+String nodeToFetchFrom;
 if (i < FETCHFROM_SRC) {
   // this is to protect very large clusters from overwhelming a single 
node
   // the first FETCHFROM_SRC nodes will be asked to fetch from this 
node.
   // it's there in  the memory now. So , it must be served fast
-  getFrom = myNodeName;

Review Comment:
   i like the nicer name



##
solr/core/src/java/org/apache/solr/filestore/NodeFileStore.java:
##
@@ -76,135 +66,54 @@ public SolrJerseyResponse getFile(String path, Boolean 
sync, String getFrom, Boo
 final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
 
 if (Boolean.TRUE.equals(sync)) {
-  try {
-fileStore.syncToAllNodes(path);
-return response;
-  } catch (IOException e) {
-throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Error 
getting file ", e);
-  }
+  ClusterFileStore.syncToAllNodes(fileStore, path);
+  return response;
 }
 
 if (path == null) {
   path = "";
 }
 final var pathCopy = path;
 if (getFrom != null) {
-  coreContainer
-  .getUpdateShardHandler()
-  .getUpdateExecutor()
-  .submit(
-  () -> {
-log.debug("Downloading file {}", pathCopy);
-try {
-  fileStore.fetch(pathCopy, getFrom);
-} catch (Exception e) {
-  log.error("Failed to download file: {}", pathCopy, e);
-}
-log.info("downloaded file: {}", pathCopy);
-  });
+  ClusterFileStore.pullFileFromNode(coreContainer, fileStore, pathCopy, 
getFrom);
   return response;
 }
 
 FileStore.FileType type = fileStore.getType(path, false);
-if (type == FileStore.FileType.NOFILE) {
-  final var fileMissingResponse =
-  instantiateJerseyResponse(FileStoreDi

Re: [PR] SOLR-17351: Decompose filestore "get file" API [solr]

[via GitHub]

gerlowskija commented on PR #3047:
URL: https://github.com/apache/solr/pull/3047#issuecomment-2603252367

   That PR uses the Java interface, and not the HTTP APIs directly, so it 
should be fine afaict.


-- 
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]

[via GitHub]

epugh commented on PR #3047:
URL: https://github.com/apache/solr/pull/3047#issuecomment-2602733589

   Does this impact #3031 ?   Since I am using `cc.getFileStore` I don't think 
so, but wanted to check.  


-- 
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