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(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
Review Comment:
this feels overly finicky logic.. why is it so hard to decide when to get
metadata? What are we missing that doens't just make this simple. A thing
has a meta thing. That should be the only way.
##########
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:
"Syncs a file via either pushing or pulling across the nodes in the Solr
cluster." ???
##########
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=<signature-of-jar>
+curl --data-binary @runtimelibs.jar -X PUT
http://localhost:8983/api/cluster/filestore/files/mypkg/1.0/myplugins.jar?sig=<signature-of-jar>
Review Comment:
maybe drops the `files`?
##########
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:
boo! Still have a GSR.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -382,16 +380,14 @@ private void distribute(FileInfo info) {
// trying to avoid the thundering herd problem when there are a very
large no:of nodes
Review Comment:
can we fix the comment? `number of `
##########
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<FileStore.FileDetails> 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:
What is the use case FOR this? Do we have one right now?
##########
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) {
+ inputStream.transferTo(os);
+ }
+ } catch (IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Error
reading file " + path);
+ }
+ },
+ false));
+ }
+
+ @SuppressWarnings("fallthrough")
+ public static FileStoreDirectoryListingResponse getMetadata(
+ FileStore.FileType type, String path, FileStore fileStore) {
+ final var dirListingResponse = new FileStoreDirectoryListingResponse();
+ if (path == null) {
+ path = "";
+ }
+
+ switch (type) {
+ case NOFILE:
+ dirListingResponse.files = Collections.singletonMap(path, null);
+ break;
+ case METADATA:
+ case FILE:
+ int idx = path.lastIndexOf('/');
+ String fileName = path.substring(idx + 1);
+ String parentPath = path.substring(0, path.lastIndexOf('/'));
+ List<FileStore.FileDetails> l = fileStore.list(parentPath, s ->
s.equals(fileName));
+
+ dirListingResponse.files =
+ Collections.singletonMap(path, l.isEmpty() ? null :
convertToResponse(l.get(0)));
+ break;
+ case DIRECTORY:
+ final var directoryContents =
+ fileStore.list(path, null).stream()
+ .map(details -> convertToResponse(details))
+ .collect(Collectors.toList());
+ dirListingResponse.files = Collections.singletonMap(path,
directoryContents);
+ break;
+ }
+
+ return dirListingResponse;
+ }
+
+ // TODO Modify the filestore implementation itself to return this object, so
conversion isn't
Review Comment:
+1 to this
##########
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:
could this ever be null? wouldn't you have an exception instead?
##########
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:
i like the more verbose path I think... Can a filestore have anything under
than files? If not, what about just `"/api/cluster/filestore"`
##########
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) {
+ inputStream.transferTo(os);
+ }
+ } catch (IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Error
reading file " + path);
+ }
+ },
+ false));
+ }
+
+ @SuppressWarnings("fallthrough")
+ public static FileStoreDirectoryListingResponse getMetadata(
+ FileStore.FileType type, String path, FileStore fileStore) {
+ final var dirListingResponse = new FileStoreDirectoryListingResponse();
+ if (path == null) {
+ path = "";
+ }
+
+ switch (type) {
+ case NOFILE:
+ dirListingResponse.files = Collections.singletonMap(path, null);
+ break;
+ case METADATA:
+ case FILE:
+ int idx = path.lastIndexOf('/');
+ String fileName = path.substring(idx + 1);
+ String parentPath = path.substring(0, path.lastIndexOf('/'));
+ List<FileStore.FileDetails> l = fileStore.list(parentPath, s ->
s.equals(fileName));
+
+ dirListingResponse.files =
+ Collections.singletonMap(path, l.isEmpty() ? null :
convertToResponse(l.get(0)));
+ break;
+ case DIRECTORY:
+ final var directoryContents =
+ fileStore.list(path, null).stream()
+ .map(details -> convertToResponse(details))
+ .collect(Collectors.toList());
+ dirListingResponse.files = Collections.singletonMap(path,
directoryContents);
+ break;
+ }
+
+ return dirListingResponse;
+ }
+
+ // TODO Modify the filestore implementation itself to return this object, so
conversion isn't
Review Comment:
i think i also ran into this in the schema designer
##########
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:
I've been a bit confused between NodeFileStore, ClusterFileStore,
DistribFileStore ;-).
--
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
