SOLR-17302: Convert remaining filestore APIs to JAX-RS by gerlowskija · Pull Request #2532 · apache/solr

gerlowskija · 2024-06-24T18:46:28Z

https://issues.apache.org/jira/browse/SOLR-17302

Description

PR #2470 converted a few file store APIs (those available under the /api/cluster path) to JAX-RS. But the PR didn't touch several other APIs, which still needed JAX-RS conversion, including:

Fetch filestore file
Fetch filestore metadata
Delete "local" filestore entry (internal API)

Solution

This PR migrates the remaining filestore APIs to JAX-RS. This migration is largely done "as-is", avoiding any cosmetic changes to the APIs. IMO cosmetic improvements are much needed here (see SOLR-17302 for more details), but I've skipped those here because despite being "experimental" v2 APIs, these APIs are the only way to access Solr's filestore, so we might need to be a little more careful about changes. I've started a discussion around this in SOLR-17302 for a hopefully wider audience to chime in.

One exception to this is the internal "local-delete" API, which is used internally by Solr. It's not documented, nor does it have any use from an end-user perspective, so changing this API seemed a little more clear-cut. This functionality can now be triggered via a localDelete parameter on the existing filestore delete API (e.g. DELETE /cluster/files/somePath.txt?localDelete=true)

We utilize "RawResponseWriter" for serializing the raw file data when fetching filestore files, to match the approach taken by the existing ZooKeeperReadAPI code.

Tests

Existing filestore tests continue to pass, as this is mostly a refactor. Additional unit tests in V2ApiUtils and elsewhere.

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.
I have added tests for my changes.

One small step in aligning all filestore APIs on the '/cluster/files/' HTTP path.

Plausibly works on the server-side, though the generated SolrRequest implementation will almost surely NOT work. Finally, this commit changes the path for these APIs to be /cluster/files (instead of the previously used `/node/files`). I need to think through the backcompat implications of this a bit more thoroughly. I probably need a version of these APIs also available at the original path.

TestDistribFileStore passes; ship it.

epugh

Great work here moving things forward... Out of curisoity, do the existing tests then cover this all with no change? amazing?

epugh · 2024-06-24T22:36:19Z

+/**
+ * V2 APIs for fetching filestore files, syncing them across nodes, or fetching related metadata.
+ *
+ * @see MergeIndexesRequestBody


Does this do the magic linking? the @see?

It's not doing anything "magic" for our code-generation, etc. - it's just something the Javadoc syntax supports.

And in this case, it's there by copy/paste error 😛 Thanks for pointing it out haha!

epugh · 2024-06-24T22:39:25Z

    METADATA
  }

-  interface FileDetails extends MapWriter {


I like this!

epugh · 2024-06-24T22:41:36Z

+    this.fileStore = fileStore;
+  }
+
+  // TODO - this single "get" operation actually supports several different chunks of functionality:


I wish we had JIRA's that referenced the "Todo" work...

It would be nice. I think the lack of a JIRA reference in this case reflects my own uncertainty about how to proceed.

In theory SOLR-17302 is already sortof meant to cover this type of "cosmetic improvement" change. That's how many of our v2 tickets are setup - to cover both JAX-RS conversion and "cosmetic changes" all in one.

But in practice I'm not sure whether it's safe/responsible to modify the appearance of these particular v2 endpoints. They're technically "experimental", but they're the only way to access the filestore - there's no v1 equivalent that we can recommend to folks.

I'm going to start a discussion on SOLR-17302 about this, which will hopefully draw some more eyes and get us some clarity on how to proceed.

Actually - small correction - apparently when I wrote up SOLR-17302 I worded it in such a way that it only covers migration to JAX-RS.

So there's no ticket (yet) for breaking up this API into more manageable chunks. I'll file a separate ticket for this and reference it here (and in the code) when its ready.

FYI: https://issues.apache.org/jira/browse/SOLR-17351

epugh · 2024-06-24T22:44:40Z

+    if (path == null) {
+      path = "";
+    }
+    FileStore.FileType typ = fileStore.getType(path, false);


This is something that carried over from the existing code, and I'm leery of getting sucked into larger improvements to that code. There's a lot that could be done there.
But I can change it since it's a small modification.

epugh · 2024-06-24T22:46:11Z

+    } 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
+      final var pathCopy2 = path;


pathCopy2???

This is something that carried over from the existing code, and I'm leery of getting sucked into larger improvements to that code. There's a lot that could be done there.
But I can change it since it's a small modification.

epugh · 2024-06-24T22:46:50Z

+        req.setParams(SolrParams.wrapDefaults(solrParams, req.getParams()));
+        rsp.add(
+            CONTENT,
+            (SolrCore.RawWriter)


epugh · 2024-06-24T22:48:03Z

+    } else if (obj != null && (obj instanceof SolrCore.RawWriter)) {
+      final var rawWriter = (SolrCore.RawWriter) obj;
+      rawWriter.write(out);
+      if (rawWriter instanceof Closeable) ((Closeable) rawWriter).close();


maybe a { and }?

This is something that carried over from existing code.

I've agreed to a few variable renames above in code that preexists this PR, but I'll probably skip this change. IMO this code could use lots of reformatting/restructuring attention, but we need to draw the line somewhere.

gerlowskija · 2024-06-25T12:51:22Z

Looks like PackageToolTest is still failing with this change, will need to figure that out prior to merging...

gerlowskija · 2024-06-26T14:52:38Z

The PackageToolTest failure actually surfaced an interesting little v2 API bug - the v2 authz code wasn't properly detecting when a request had been authenticated as the special PKI principal '$'. This PR now fixes that bug as well, using code similar to what we use in the v1 case. With this fixed I'm marking the PR as "ready for review"!

epugh · 2024-06-26T21:16:04Z

The PackageToolTest failure actually surfaced an interesting little v2 API bug - the v2 authz code wasn't properly detecting when a request had been authenticated as the special PKI principal '$'. This PR now fixes that bug as well, using code similar to what we use in the v1 case. With this fixed I'm marking the PR as "ready for review"!

Gnarly little fix there!

epugh

Some great work here!

This migrates the remainder of Solr's "filestore" APIs to the JAX-RS framework. No cosmetic changes were made in the process, with the small exception of folding the internal "local delete" functionality into the existing delete API using a new `localDelete` boolean query param.

gerlowskija added 3 commits June 21, 2024 07:40

SOLR-17302: Merge filestore local/cluster delete

cf2328c

One small step in aligning all filestore APIs on the '/cluster/files/' HTTP path.

WIP: Swap get-path back to /api/node

537f0c4

TestDistribFileStore passes; ship it.

github-actions bot added tests cat:search cat:api cat:packagemanager labels Jun 24, 2024

gerlowskija added 3 commits June 24, 2024 15:34

Minor cleanup, pt 1

d87ebb7

Add unit test

477008e

Attempt removal of MapWriter from FileDetails

7d0ffc7

epugh reviewed Jun 24, 2024

View reviewed changes

Minor test fix

8851993

gerlowskija marked this pull request as ready for review June 26, 2024 14:52

gerlowskija added 2 commits June 26, 2024 11:35

Omit 'getFile' endpoint from codegen

e963b27

Address review comments, rd 1

ce4bcb9

github-actions bot added the client:solrj label Jun 26, 2024

epugh approved these changes Jun 26, 2024

View reviewed changes

Add JIRA reference to TODO comment

732ac8b

gerlowskija merged commit d0aad69 into apache:main Jul 1, 2024

gerlowskija deleted the SOLR-17302-convert-local-delete-to-jax-rs branch July 1, 2024 14:48

Conversation

gerlowskija commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerlowskija commented Jun 25, 2024

Uh oh!

gerlowskija commented Jun 26, 2024

Uh oh!

epugh commented Jun 26, 2024

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gerlowskija commented Jun 24, 2024 •

edited

Loading