SOLR-17302: Convert /cluster filestore APIs to JAX-RS #2470

gerlowskija · 2024-05-20T20:07:12Z

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:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.

This commit moves some of Solr's "filestore" APIs, those located under the `/api/cluster/files` path, to JAX-RS. Still needs some tidying up and sanity-checking, but running a few of the relevant tests pass which is promising given earlier results.

epugh

Looks like some great work here! I tagged some places where it would be nice ot have more descriptive variable names, however I suspect you are trying to minimize change. Do we still need DistribFileStore in a world with ClusterFileStore?

epugh · 2024-05-21T00:13:56Z

solr/api/src/java/org/apache/solr/client/api/endpoint/ClusterFileStoreApis.java

+
+@Path("/cluster")
+public interface ClusterFileStoreApis {
+  // TODO Better understand the purpose of the 'sig' parameter and improve docs here.


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

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

epugh · 2024-05-21T00:17:47Z

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


Sigh I hate you _ methods and objects!

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 😦

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

epugh · 2024-05-21T00:18:43Z

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

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

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

solr/core/src/java/org/apache/solr/filestore/FileStoreUtils.java

solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java

solr/core/src/java/org/apache/solr/pkg/PackageAPI.java

epugh · 2024-05-21T00:23:24Z

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

Do we need DistribFileStore when we have ClusterFileStore?

gerlowskija · 2024-05-21T01:06:11Z

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:

FileStore is an interface that defines the operations that Solr's file-store has to support.
DistribFileStore is the lone implementation of the FileStore interface.
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!?

epugh · 2024-05-21T01:22:02Z

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!

gerlowskija · 2024-05-21T10:41:23Z

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.

…axrs

epugh · 2024-05-24T18:04:25Z

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?

epugh · 2024-05-24T18:09:46Z

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.

gerlowskija · 2024-05-28T16:33:07Z

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

This commit moves some of Solr's "filestore" APIs (those located under the `/api/cluster/files` path) to JAX-RS.

gerlowskija · 2024-05-30T14:07:36Z

I've since noticed two problems with this PR:

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.
This breaks a test that @AndreyBozhko had in-flight on PR SOLR-17304: PKG_VERSIONS not honored when loading the schema plugins #2471.

I'm going to revert until I've had a chance to fix (1) and so that Andrey isn't blocked.

This reverts commit 8c5ae1b.

This reverts commit a3a0253.

This commit moves some of Solr's "filestore" APIs (those located under the `/api/cluster/files` path) to JAX-RS.

gerlowskija added 4 commits May 20, 2024 15:36

Misc NOCOMMIT cleanups

12a4d70

Misc cleanup 2

64c3923

misc cleanup 3

295a6be

github-actions bot added tests cat:api cat:packagemanager labels May 20, 2024

epugh reviewed May 21, 2024

View reviewed changes

gerlowskija added 2 commits May 21, 2024 06:46

Address review comments, rd 1

b04ceac

Merge branch 'main' into SOLR-17302-convert-filestore-write-apis-to-j…

56ae9d4

…axrs

gerlowskija merged commit 8c5ae1b into apache:main May 28, 2024
3 checks passed

gerlowskija deleted the SOLR-17302-convert-filestore-write-apis-to-jaxrs branch May 28, 2024 18:12

gerlowskija added a commit that referenced this pull request May 28, 2024

SOLR-17302: Convert /cluster filestore APIs to JAX-RS (#2470)

a3a0253

This commit moves some of Solr's "filestore" APIs (those located under the `/api/cluster/files` path) to JAX-RS.

gerlowskija mentioned this pull request May 30, 2024

SOLR-17304: PKG_VERSIONS not honored when loading the schema plugins #2471

Merged

7 tasks

gerlowskija added a commit that referenced this pull request May 30, 2024

Revert "SOLR-17302: Convert /cluster filestore APIs to JAX-RS (#2470)"

8814481

This reverts commit 8c5ae1b.

gerlowskija added a commit that referenced this pull request May 30, 2024

Revert "SOLR-17302: Convert /cluster filestore APIs to JAX-RS (#2470)"

fde2023

This reverts commit a3a0253.

gerlowskija added a commit to gerlowskija/solr that referenced this pull request Jun 4, 2024

SOLR-17302: Convert /cluster filestore APIs to JAX-RS (apache#2470)

e0e5ecc

This commit moves some of Solr's "filestore" APIs (those located under the `/api/cluster/files` path) to JAX-RS.

gerlowskija mentioned this pull request Jun 7, 2024

SOLR-17302: Convert /cluster filestore APIs to JAX-RS (reinstating after revert) #2507

Merged

6 tasks

gerlowskija mentioned this pull request Jun 24, 2024

SOLR-17302: Convert remaining filestore APIs to JAX-RS #2532

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17302: Convert /cluster filestore APIs to JAX-RS #2470

SOLR-17302: Convert /cluster filestore APIs to JAX-RS #2470

gerlowskija commented May 20, 2024 •

edited

Loading

epugh left a comment

epugh May 21, 2024

epugh May 21, 2024

gerlowskija May 21, 2024

epugh May 21, 2024

epugh May 21, 2024

gerlowskija May 21, 2024

epugh May 21, 2024

gerlowskija commented May 21, 2024

epugh commented May 21, 2024

gerlowskija commented May 21, 2024

epugh commented May 24, 2024

epugh commented May 24, 2024

gerlowskija commented May 28, 2024

gerlowskija commented May 30, 2024

SOLR-17302: Convert /cluster filestore APIs to JAX-RS #2470

SOLR-17302: Convert /cluster filestore APIs to JAX-RS #2470

Conversation

gerlowskija commented May 20, 2024 • edited Loading

Description

Solution

Tests

Checklist

epugh left a comment

Choose a reason for hiding this comment

epugh May 21, 2024

Choose a reason for hiding this comment

epugh May 21, 2024

Choose a reason for hiding this comment

gerlowskija May 21, 2024

Choose a reason for hiding this comment

epugh May 21, 2024

Choose a reason for hiding this comment

epugh May 21, 2024

Choose a reason for hiding this comment

gerlowskija May 21, 2024

Choose a reason for hiding this comment

epugh May 21, 2024

Choose a reason for hiding this comment

gerlowskija commented May 21, 2024

epugh commented May 21, 2024

gerlowskija commented May 21, 2024

epugh commented May 24, 2024

epugh commented May 24, 2024

gerlowskija commented May 28, 2024

gerlowskija commented May 30, 2024

gerlowskija commented May 20, 2024 •

edited

Loading