Details

    • Sub-task
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 9.7
    • v2 API

    Description

      As mentioned on SOLR-15781, the v2 API currently has an experimental designation, and the community has expressed an interest in using this period to update our v2 endpoints to be more REST-ful and consistent. The current plan is to follow the specific changes laid out in this spreadsheet, though of course nothing there is set in stone and there are still warts to be worked out.

      While we're touching the code for these endpoints, we should also convert them to JAX-RS framework definitions. (This was initially tracked as a separate effort - see SOLR-16370 - but the edit that were required ended up overlapping so significantly with the "cosmetic" improvements here that in practice it almost always makes sense to do the two together.)

      This ticket plans to tackle making the changes required for Solr's "filestore" APIs. Most of these APIs are fine cosmetically as-is, though this might be a good opportunity to iron out a few warts (particularly in how the "/api/node" and "/api/cluster" paths are used inconsistently, unless there's a rationale there that I'm missing...)

      JAX-RS Conversion

      API Name Form Status Volunteer
      Upload File PUT /api/cluster/files/foo.txt Done Jason
      Delete File DELETE /api/cluster/files/foo.txt Done Jason
      Delete File (Local) DELETE /api/cluster/files/foo.txt?localDelete=true Done Jason
      Fetch File GET /api/note/files/foo.txt Done Jason
      Fetch File Metadata GET /api/note/files/foo.txt?meta=true Done Jason

      Some helpful links related to these changes these changes. Should help get any interested newcomers started!

      Attachments

        Activity

          githubbot ASF GitHub Bot logged work - 20/May/24 20:07
          • Time Spent:
            10m
             
            gerlowskija opened a new pull request, #2470:
            URL: https://github.com/apache/solr/pull/2470

               https://issues.apache.org/jira/browse/SOLR-17302
               
               # Description
               
               Solr has slowly been modifying its v2 APIs to align around a more REST-ful model, implemented using the JAX-RS framework. Many APIs have already been through this re-alignment, but many more remain including Solr's filestore APIs (used internally by package store).
               
               # Solution
               
               This commit migrates several of the filestore APIs, namely those under the `/cluster/files` path to use the JAX-RS framework. The APIs themselves are unchanged from an end-user perspective.
               
               Other endpoints, particularly the "fetch filestore entry" (`GET /api/node/files/somepath.txt`) and "delete local filestore entry" (`DELETE /api/node/files/somepath.txt`) are not attempted in this PR, but left for another.
               
               # Tests
               
               Existing filestore and package-store tests continue to pass.
               
               # 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.
               - [x] I have created a Jira issue and added the issue ID to my pull request title.
               - [x] 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.
               - [ ] I have run `./gradlew check`.
               


          githubbot ASF GitHub Bot logged work - 21/May/24 00:25
          • Time Spent:
            10m
             
            epugh commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1607425946


            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreAPI.java:
            ##########


            Review Comment:
               I sort of thought maybe `FileStoreAPI.java` would be totally removed in favour of the new classes?



            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########
            @@ -217,7 +217,7 @@ private boolean fetchFileFromNodeAndPersist(String fromNode) {
                 }
             
                 boolean fetchFromAnyNode() {
            - ArrayList<String> l = coreContainer.getFileStoreAPI().shuffledNodes();
            + ArrayList<String> l = FileStoreUtils.shuffledNodes(coreContainer);

            Review Comment:
               i know you are worried about scope, but maybe not `l`? `liveNodes`?



            ##########
            solr/core/src/java/org/apache/solr/pkg/PackageAPI.java:
            ##########
            @@ -254,7 +250,7 @@ public void refresh(PayloadObj<String> payload) {
                   }
                   // first refresh my own
                   packageLoader.notifyListeners(p);
            - for (String s : coreContainer.getFileStoreAPI().shuffledNodes()) {
            + for (String s : FileStoreUtils.shuffledNodes(coreContainer)) {

            Review Comment:
               better name on what is returned...?



            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreUtils.java:
            ##########
            @@ -0,0 +1,145 @@
            +/*
            + * 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.filestore;
            +
            +import java.io.IOException;
            +import java.io.InputStream;
            +import java.lang.invoke.MethodHandles;
            +import java.util.ArrayList;
            +import java.util.Collections;
            +import java.util.List;
            +import java.util.Map;
            +import java.util.Set;
            +import java.util.function.Consumer;
            +import java.util.function.Supplier;
            +import org.apache.solr.common.SolrException;
            +import org.apache.solr.core.BlobRepository;
            +import org.apache.solr.core.CoreContainer;
            +import org.apache.solr.util.CryptoKeys;
            +import org.slf4j.Logger;
            +import org.slf4j.LoggerFactory;
            +
            +/** Common utilities used by filestore-related code. */
            +public class FileStoreUtils {
            + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
            +
            + /** get a list of nodes randomly shuffled * @lucene.internal */
            + public static ArrayList<String> shuffledNodes(CoreContainer coreContainer) {
            + Set<String> liveNodes =
            + coreContainer.getZkController().getZkStateReader().getClusterState().getLiveNodes();
            + ArrayList<String> l = new ArrayList<>(liveNodes);
            + l.remove(coreContainer.getZkController().getNodeName());
            + Collections.shuffle(l, BlobRepository.RANDOM);
            + return l;
            + }
            +
            + public static void validateFiles(
            + FileStore fileStore, List<String> files, boolean validateSignatures, Consumer<String> errs) {
            + for (String path : files) {
            + try {
            + FileStore.FileType type = fileStore.getType(path, true);
            + if (type != FileStore.FileType.FILE) {
            + errs.accept("No such file: " + path);
            + continue;
            + }
            +
            + fileStore.get(
            + path,
            + entry -> {
            + if (entry.getMetaData().signatures == null
            + || entry.getMetaData().signatures.isEmpty()) {
            + errs.accept(path + " has no signature");
            + return;
            + }
            + if (validateSignatures) {
            + try {
            + fileStore.refresh(ClusterFileStore.KEYS_DIR);
            + validate(fileStore, entry.meta.signatures, entry, false);
            + } catch (Exception e) {
            + log.error("Error validating package artifact", e);
            + errs.accept(e.getMessage());
            + }
            + }
            + },
            + false);
            + } catch (Exception e) {
            + log.error("Error reading file ", e);
            + errs.accept("Error reading file " + path + " " + e.getMessage());
            + }
            + }
            + }
            +
            + /**
            + * Validate a file for signature
            + *
            + * @param sigs the signatures. atleast one should succeed

            Review Comment:
               `@param signatures The Signatures, at least one should succeed` ?



            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########
            @@ -634,7 +635,7 @@ public Map<String, byte[]> getKeys() throws IOException {
               // reads local keys file
               private static Map<String, byte[]> _getKeys(Path solrHome) throws IOException {
                 Map<String, byte[]> result = new HashMap<>();
            - Path keysDir = _getRealPath(FileStoreAPI.KEYS_DIR, solrHome);
            + Path keysDir = _getRealPath(ClusterFileStore.KEYS_DIR, solrHome);

            Review Comment:
               Sigh I hate you `_` methods and objects!



            ##########
            solr/api/src/java/org/apache/solr/client/api/endpoint/ClusterFileStoreApis.java:
            ##########
            @@ -0,0 +1,56 @@
            +/*
            + * 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.client.api.endpoint;
            +
            +import io.swagger.v3.oas.annotations.Operation;
            +import io.swagger.v3.oas.annotations.Parameter;
            +import io.swagger.v3.oas.annotations.parameters.RequestBody;
            +import jakarta.ws.rs.DELETE;
            +import jakarta.ws.rs.PUT;
            +import jakarta.ws.rs.Path;
            +import jakarta.ws.rs.PathParam;
            +import jakarta.ws.rs.QueryParam;
            +import java.io.InputStream;
            +import java.util.List;
            +import org.apache.solr.client.api.model.SolrJerseyResponse;
            +import org.apache.solr.client.api.model.UploadToFileStoreResponse;
            +
            +@Path("/cluster")
            +public interface ClusterFileStoreApis {
            + // TODO Better understand the purpose of the 'sig' parameter and improve docs here.

            Review Comment:
               Yeah... There is some signing of stuff, and it probably should be a more global capability for managing uploading things. Maybe you "sign" a configset for example. Or treat a file the same ways we treat a configset?



            ##########
            solr/api/src/java/org/apache/solr/client/api/endpoint/ClusterFileStoreApis.java:
            ##########
            @@ -0,0 +1,56 @@
            +/*
            + * 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.client.api.endpoint;
            +
            +import io.swagger.v3.oas.annotations.Operation;
            +import io.swagger.v3.oas.annotations.Parameter;
            +import io.swagger.v3.oas.annotations.parameters.RequestBody;
            +import jakarta.ws.rs.DELETE;
            +import jakarta.ws.rs.PUT;
            +import jakarta.ws.rs.Path;
            +import jakarta.ws.rs.PathParam;
            +import jakarta.ws.rs.QueryParam;
            +import java.io.InputStream;
            +import java.util.List;
            +import org.apache.solr.client.api.model.SolrJerseyResponse;
            +import org.apache.solr.client.api.model.UploadToFileStoreResponse;
            +
            +@Path("/cluster")
            +public interface ClusterFileStoreApis {
            + // TODO Better understand the purpose of the 'sig' parameter and improve docs here.
            + @PUT
            + @Operation(
            + summary = "Upload a file to the filestore.",
            + tags = {"file-store"})
            + @Path("/files{filePath:.+}")
            + UploadToFileStoreResponse uploadFile(
            + @Parameter(description = "File store path") @PathParam("filePath") String filePath,
            + @Parameter(description = "Signature(s) for the file being uploaded") @QueryParam("sig")

            Review Comment:
               Should `sig` be `signature` ?



            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreUtils.java:
            ##########
            @@ -0,0 +1,145 @@
            +/*
            + * 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.filestore;
            +
            +import java.io.IOException;
            +import java.io.InputStream;
            +import java.lang.invoke.MethodHandles;
            +import java.util.ArrayList;
            +import java.util.Collections;
            +import java.util.List;
            +import java.util.Map;
            +import java.util.Set;
            +import java.util.function.Consumer;
            +import java.util.function.Supplier;
            +import org.apache.solr.common.SolrException;
            +import org.apache.solr.core.BlobRepository;
            +import org.apache.solr.core.CoreContainer;
            +import org.apache.solr.util.CryptoKeys;
            +import org.slf4j.Logger;
            +import org.slf4j.LoggerFactory;
            +
            +/** Common utilities used by filestore-related code. */
            +public class FileStoreUtils {
            + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
            +
            + /** get a list of nodes randomly shuffled * @lucene.internal */

            Review Comment:
               why `lucene.internal`?



            ##########
            solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:
            ##########
            @@ -302,7 +303,7 @@ public static String getCollectionParamsPath(String collection) {
               }
             
               public static void uploadKey(byte[] bytes, String path, Path home) throws IOException {
            - FileStoreAPI.MetaData meta = FileStoreAPI._createJsonMetaData(bytes, null);
            + FileStoreAPI.MetaData meta = ClusterFileStore._createJsonMetaData(bytes, null);

            Review Comment:
               guessing minimizing change, ubt maybe we can remove `_` ?



            ##########
            solr/core/src/java/org/apache/solr/pkg/PackageAPI.java:
            ##########
            @@ -426,7 +422,7 @@ private void syncToVersion(int expectedVersion) {
               }
             
               void notifyAllNodesToSync(int expected) {
            - for (String s : coreContainer.getFileStoreAPI().shuffledNodes()) {
            + for (String s : FileStoreUtils.shuffledNodes(coreContainer)) {

            Review Comment:
               LIke wise? what is the string...



            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########


            Review Comment:
               Do we need DistribFileStore when we have ClusterFileStore?



            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########
            @@ -217,7 +217,7 @@ private boolean fetchFileFromNodeAndPersist(String fromNode) {
                 }
             
                 boolean fetchFromAnyNode() {
            - ArrayList<String> l = coreContainer.getFileStoreAPI().shuffledNodes();
            + ArrayList<String> l = FileStoreUtils.shuffledNodes(coreContainer);

            Review Comment:
               or `nodes`?



          githubbot ASF GitHub Bot logged work - 21/May/24 01:06
          • Time Spent:
            10m
             
            gerlowskija commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2121513823

               Thanks for the quick review Eric!
               
               > I tagged some places where it would be nice ot have more descriptive variable names, however I suspect you are trying to minimize change
               
               Your suspicion is correct. This PR aims to convert the API(s) to JAX-RS without delving too deeply into cleaning up the underlying implementation. I think some of the stuff you commented on is innocuous enough that I can be tempted into making a few cleanups though, particularly some of the naming and javadoc/comment issues.
               
               > Do we still need DistribFileStore in a world with ClusterFileStore?
               
               Yes, we do. There's some naming-related confusion going on here that I've definitely caused:
               
               1. `FileStore` is an interface that defines the operations that Solr's file-store has to support.
               2. `DistribFileStore` is the lone implementation of the `FileStore` interface.
               3. `ClusterFileStore` is the entrypoint for the relevant v2 APIs.
               
               `ClusterFileStore` is, on its surface, a pretty bad naming choice given the pre-existing `FileStore` interface. It 100% appears to be an implementation of that interface, but it's not.
               
               I do have an excuse, though it may be a bad one 😛 I chose `ClusterFileStore` for the v2 API class name to be consistent with a naming convention we've had since creating the "api" gradle module earlier this year. (i.e. interfaces in "api" module are `FooApi`, with classes in solr-core being `Foo`).
               
               The filestore (for reasons I still don't really understand) makes APIs available under two paths: `/api/cluster/files` and `/api/node/files`. So I chose `ClusterFileStoreApi`/`ClusterFileStore` partially in an attempt to distinguish between these two sets of filestore APIs, and partially to maintain consistency with the existing naming convention.
               
               The end result is clearly confusing; very open if anyone has other suggestions that'd avoid the ambiguity and confusion!?


          githubbot ASF GitHub Bot logged work - 21/May/24 01:22
          • Time Spent:
            10m
             
            epugh commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2121526630

               If `DistribFileStore` is the lone implementation of `FileStore`, then do we need `FileStore`? I never liked the whole "Let me have an interface for everything" cause it doubles the files to navigate through. In the future, if we need a `FileStore` implementation because some wrote `NonDistribFileStore` then we can make it then... We aren't C code, we don't need a seperate `.h` file for every `.cpp` file!


          githubbot ASF GitHub Bot logged work - 21/May/24 10:13
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608034094


            ##########
            solr/api/src/java/org/apache/solr/client/api/endpoint/ClusterFileStoreApis.java:
            ##########
            @@ -0,0 +1,56 @@
            +/*
            + * 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.client.api.endpoint;
            +
            +import io.swagger.v3.oas.annotations.Operation;
            +import io.swagger.v3.oas.annotations.Parameter;
            +import io.swagger.v3.oas.annotations.parameters.RequestBody;
            +import jakarta.ws.rs.DELETE;
            +import jakarta.ws.rs.PUT;
            +import jakarta.ws.rs.Path;
            +import jakarta.ws.rs.PathParam;
            +import jakarta.ws.rs.QueryParam;
            +import java.io.InputStream;
            +import java.util.List;
            +import org.apache.solr.client.api.model.SolrJerseyResponse;
            +import org.apache.solr.client.api.model.UploadToFileStoreResponse;
            +
            +@Path("/cluster")
            +public interface ClusterFileStoreApis {
            + // TODO Better understand the purpose of the 'sig' parameter and improve docs here.
            + @PUT
            + @Operation(
            + summary = "Upload a file to the filestore.",
            + tags = {"file-store"})
            + @Path("/files{filePath:.+}")
            + UploadToFileStoreResponse uploadFile(
            + @Parameter(description = "File store path") @PathParam("filePath") String filePath,
            + @Parameter(description = "Signature(s) for the file being uploaded") @QueryParam("sig")

            Review Comment:
               Probably, or `signatures` - but I'm going to avoid cosmetic changes to the API (in this PR) as making those would drag in changes to other places the API is called. I do aim to update those spots (to remove GenericSolrRequest usage, as we've discussed), just not trying to do it in this PR.



          githubbot ASF GitHub Bot logged work - 21/May/24 10:21
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608050226


            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########
            @@ -217,7 +217,7 @@ private boolean fetchFileFromNodeAndPersist(String fromNode) {
                 }
             
                 boolean fetchFromAnyNode() {
            - ArrayList<String> l = coreContainer.getFileStoreAPI().shuffledNodes();
            + ArrayList<String> l = FileStoreUtils.shuffledNodes(coreContainer);

            Review Comment:
               I went with `nodesToAttemptFetchFrom`



          githubbot ASF GitHub Bot logged work - 21/May/24 10:23
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608053404


            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########
            @@ -634,7 +635,7 @@ public Map<String, byte[]> getKeys() throws IOException {
               // reads local keys file
               private static Map<String, byte[]> _getKeys(Path solrHome) throws IOException {
                 Map<String, byte[]> result = new HashMap<>();
            - Path keysDir = _getRealPath(FileStoreAPI.KEYS_DIR, solrHome);
            + Path keysDir = _getRealPath(ClusterFileStore.KEYS_DIR, solrHome);

            Review Comment:
               Agreed, I don't love the convention either. There's several of these in DistribFileStore though - enough that I'm worried renaming will bloat this PR too much 😦



          githubbot ASF GitHub Bot logged work - 21/May/24 10:25
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608056211


            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreAPI.java:
            ##########


            Review Comment:
               Ultimately it will, but for scope reasons this PR only converts some of FileStoreAPI API's to JAX-RS.
               
               I initially tried to do all the APIs at once, but the change got a bit unmanageable as time went on so I've split it into two PRs (of which this is the first).



          githubbot ASF GitHub Bot logged work - 21/May/24 10:27
          • Time Spent:
            10m
             
            epugh commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608058108


            ##########
            solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
            ##########
            @@ -634,7 +635,7 @@ public Map<String, byte[]> getKeys() throws IOException {
               // reads local keys file
               private static Map<String, byte[]> _getKeys(Path solrHome) throws IOException {
                 Map<String, byte[]> result = new HashMap<>();
            - Path keysDir = _getRealPath(FileStoreAPI.KEYS_DIR, solrHome);
            + Path keysDir = _getRealPath(ClusterFileStore.KEYS_DIR, solrHome);

            Review Comment:
               Was it "errorprone" that we used to help us clean up our code and enforce some conventions? For another JIRA, see if errorprone would flag any "_" methods for example. We *do* need to make all our conventions enforced via automated check, there are too many people over too many years to otherwise keep code standards intact...



          githubbot ASF GitHub Bot logged work - 21/May/24 10:27
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608058864


            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreUtils.java:
            ##########
            @@ -0,0 +1,145 @@
            +/*
            + * 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.filestore;
            +
            +import java.io.IOException;
            +import java.io.InputStream;
            +import java.lang.invoke.MethodHandles;
            +import java.util.ArrayList;
            +import java.util.Collections;
            +import java.util.List;
            +import java.util.Map;
            +import java.util.Set;
            +import java.util.function.Consumer;
            +import java.util.function.Supplier;
            +import org.apache.solr.common.SolrException;
            +import org.apache.solr.core.BlobRepository;
            +import org.apache.solr.core.CoreContainer;
            +import org.apache.solr.util.CryptoKeys;
            +import org.slf4j.Logger;
            +import org.slf4j.LoggerFactory;
            +
            +/** Common utilities used by filestore-related code. */
            +public class FileStoreUtils {
            + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
            +
            + /** get a list of nodes randomly shuffled * @lucene.internal */

            Review Comment:
               Idk - it was in the Javadoc when I copied this method out of FileStoreAPI.
               
               We're not in SolrJ so the original author probably wasn't worried about signaling backcompat - my guess was that 'lucene.internal' was more about signaling that this was an implementation detail and that folks should be leery of calling the method?
               
               I'll remove it.



          githubbot ASF GitHub Bot logged work - 21/May/24 10:28
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608058864


            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreUtils.java:
            ##########
            @@ -0,0 +1,145 @@
            +/*
            + * 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.filestore;
            +
            +import java.io.IOException;
            +import java.io.InputStream;
            +import java.lang.invoke.MethodHandles;
            +import java.util.ArrayList;
            +import java.util.Collections;
            +import java.util.List;
            +import java.util.Map;
            +import java.util.Set;
            +import java.util.function.Consumer;
            +import java.util.function.Supplier;
            +import org.apache.solr.common.SolrException;
            +import org.apache.solr.core.BlobRepository;
            +import org.apache.solr.core.CoreContainer;
            +import org.apache.solr.util.CryptoKeys;
            +import org.slf4j.Logger;
            +import org.slf4j.LoggerFactory;
            +
            +/** Common utilities used by filestore-related code. */
            +public class FileStoreUtils {
            + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
            +
            + /** get a list of nodes randomly shuffled * @lucene.internal */

            Review Comment:
               Idk - it was in the Javadoc when I copied this method out of FileStoreAPI.
               
               We're not in SolrJ so the original author probably wasn't worried about signaling backcompat - my guess was that 'lucene.internal' was more about signaling that this was an implementation detail and that folks should be leery of calling the method?
               
               It probably doesn't make sense though, given this method is now in a Utils class, and had grown to be pretty widely called even before this PR. Will remove.



          githubbot ASF GitHub Bot logged work - 21/May/24 10:29
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608060496


            ##########
            solr/core/src/java/org/apache/solr/filestore/FileStoreUtils.java:
            ##########
            @@ -0,0 +1,145 @@
            +/*
            + * 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.filestore;
            +
            +import java.io.IOException;
            +import java.io.InputStream;
            +import java.lang.invoke.MethodHandles;
            +import java.util.ArrayList;
            +import java.util.Collections;
            +import java.util.List;
            +import java.util.Map;
            +import java.util.Set;
            +import java.util.function.Consumer;
            +import java.util.function.Supplier;
            +import org.apache.solr.common.SolrException;
            +import org.apache.solr.core.BlobRepository;
            +import org.apache.solr.core.CoreContainer;
            +import org.apache.solr.util.CryptoKeys;
            +import org.slf4j.Logger;
            +import org.slf4j.LoggerFactory;
            +
            +/** Common utilities used by filestore-related code. */
            +public class FileStoreUtils {
            + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
            +
            + /** get a list of nodes randomly shuffled * @lucene.internal */
            + public static ArrayList<String> shuffledNodes(CoreContainer coreContainer) {
            + Set<String> liveNodes =
            + coreContainer.getZkController().getZkStateReader().getClusterState().getLiveNodes();
            + ArrayList<String> l = new ArrayList<>(liveNodes);
            + l.remove(coreContainer.getZkController().getNodeName());
            + Collections.shuffle(l, BlobRepository.RANDOM);
            + return l;
            + }
            +
            + public static void validateFiles(
            + FileStore fileStore, List<String> files, boolean validateSignatures, Consumer<String> errs) {
            + for (String path : files) {
            + try {
            + FileStore.FileType type = fileStore.getType(path, true);
            + if (type != FileStore.FileType.FILE) {
            + errs.accept("No such file: " + path);
            + continue;
            + }
            +
            + fileStore.get(
            + path,
            + entry -> {
            + if (entry.getMetaData().signatures == null
            + || entry.getMetaData().signatures.isEmpty()) {
            + errs.accept(path + " has no signature");
            + return;
            + }
            + if (validateSignatures) {
            + try {
            + fileStore.refresh(ClusterFileStore.KEYS_DIR);
            + validate(fileStore, entry.meta.signatures, entry, false);
            + } catch (Exception e) {
            + log.error("Error validating package artifact", e);
            + errs.accept(e.getMessage());
            + }
            + }
            + },
            + false);
            + } catch (Exception e) {
            + log.error("Error reading file ", e);
            + errs.accept("Error reading file " + path + " " + e.getMessage());
            + }
            + }
            + }
            +
            + /**
            + * Validate a file for signature
            + *
            + * @param sigs the signatures. atleast one should succeed

            Review Comment:
               Done



          githubbot ASF GitHub Bot logged work - 21/May/24 10:30
          • Time Spent:
            10m
             
            gerlowskija commented on code in PR #2470:
            URL: https://github.com/apache/solr/pull/2470#discussion_r1608062603


            ##########
            solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:
            ##########
            @@ -302,7 +303,7 @@ public static String getCollectionParamsPath(String collection) {
               }
             
               public static void uploadKey(byte[] bytes, String path, Path home) throws IOException {
            - FileStoreAPI.MetaData meta = FileStoreAPI._createJsonMetaData(bytes, null);
            + FileStoreAPI.MetaData meta = ClusterFileStore._createJsonMetaData(bytes, null);

            Review Comment:
               I've opted out of renaming this '_' methods, as there are several of them and I'm afraid of bloating this PR's diff.



          githubbot ASF GitHub Bot logged work - 21/May/24 10:41
          • Time Spent:
            10m
             
            gerlowskija commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2122326702

               > If DistribFileStore is the lone implementation of FileStore, then do we need FileStore?
               
               I suspect the intention was that having an interface leaves the door open to adding a Solr-standalone filestore implementation at some point if someone is motivated enough.
               
               I don't feel strongly on whether the interface remains or not. Maybe that's worth cleaning up at some point, but IMO it's not necessary here since this PR isn't changing anything about how the FileStore is implemented, just some APIs around it.


          githubbot ASF GitHub Bot logged work - 24/May/24 18:04
          • Time Spent:
            10m
             
            epugh commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2130102058

               > > If DistribFileStore is the lone implementation of FileStore, then do we need FileStore?
               >
               > I suspect the intention was that having an interface leaves the door open to adding a Solr-standalone filestore implementation at some point if someone is motivated enough.
               >
               > I don't feel strongly on whether the interface remains or not. Maybe that's worth cleaning up at some point, but IMO it's not necessary here since this PR isn't changing anything about how the FileStore is implemented, just some APIs around it.
               
               Assuming we do it as a seperate JIRA, what do you think of renaming DistribFileStore to FileStore and removing the interface?


          githubbot ASF GitHub Bot logged work - 24/May/24 18:09
          • Time Spent:
            10m
             
            epugh commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2130110164

               Reading back some comments, `ClusterFileStore` is the entry point that implements `ClusterFileStoreAPI`, is there an argument that `ClusterFileStore` should do everything `DistribFileStore` does?
               
               So honestly, since the goal of this JIRA is just to go get /cluster over to jax-rs, we should probably tee up a seperate JIRA to try and untangle these classes post this one being committed.... Heck, and then maybe we take another whack at BlobHandler and maybe have a proper interface that works across all the implementations! I look forward to this being merged.


          githubbot ASF GitHub Bot logged work - 28/May/24 16:33
          • Time Spent:
            10m
             
            gerlowskija commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2135679557

               > is there an argument that ClusterFileStore should do everything DistribFileStore does?
               
               I guess? Idk about "do everything", but ClusterFileStore/ClusterFileStoreApi will eventually _expose everything_ that DistribFileStore supports.
               
               And I don't think that's a bad thing - it's an intentional "separation of concerns" thing. "ClusterFileStore"/"ClusterFileStoreApi" define the HTTP interface an end-user sees, and some of the API concerns around that. "DistribFileStore" is concerned with how we actually implement the "Filestore" idea in a SolrCloud context. In theory, we could rewrite the filestore entirely without having to touch the API code, and that's a good thing!
               
               The names are a mess and that definitely needs straightened out, but even in the long term IMO each of these classes has a distinct purpose that make them worth keeping around. Totally agree about the other TODOs you mentioned in your comment above though - it'd be great if we could migrate all the BlobHandler usage onto the filestore. (Though as you mentioned - those are different tickets for sure.)


          githubbot ASF GitHub Bot logged work - 28/May/24 18:12
          githubbot ASF GitHub Bot logged work - 30/May/24 14:07
          • Time Spent:
            10m
             
            gerlowskija commented on PR #2470:
            URL: https://github.com/apache/solr/pull/2470#issuecomment-2139638871

               I've since noticed two problems with this PR:
               
               1. The generated SolrRequest class for uploading files doesn't handle the request body correctly, making it largely unusable. We can fix this by modifying the JAX-RS annotations to be similar to those used in ZooKeeperReadAPI.
               2. This breaks a test that @AndreyBozhko had in-flight on PR #2471.
               
               I'm going to revert until I've had a chance to fix (1) and so that Andrey isn't blocked.


          githubbot ASF GitHub Bot logged work - 07/Jun/24 13:51
          githubbot ASF GitHub Bot logged work - 07/Jun/24 14:10
          • Time Spent:
            10m
             
            gerlowskija commented on PR #2507:
            URL: https://github.com/apache/solr/pull/2507#issuecomment-2154928517

               With the exception of the `api.mustache` changes, everything in this PR has already been reviewed in #2470 , so I'm going to merge it fairly quickly so that it can be in the weekend's test runs. (I'm still happy to address feedback here, if anyone has comments)


          githubbot ASF GitHub Bot logged work - 07/Jun/24 14:12

          People

            gerlowskija Jason Gerlowski
            gerlowskija Jason Gerlowski
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 3h 40m
                3h 40m