[SOLR-15737]: Create v2 equivalent of v1 'CREATESNAPSHOT', 'LISTSNAPSHOT' and 'DELETESNAPSHOT' (collection level)

[jdurham2843]

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

Description

This PR contains the V2 API implementations of CREATE, LIST, and DELETE collection snapshot resources.

Solution

I created Jersey implementations of the existing CREATE, LIST, and DELETE collection snapshot resources. I then refactored the existing V1 endpoints to call the new V2 resources.

Tests

TODO: I plan to write tests that excercise the V2 resources.

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.
• I have added documentation for the Reference Guide

[jdurham2843]

TODO:

1. Add javadoc documentation and possibly reference guide documentation.
2. Add tests for the new Jersey Resources.

[gerlowskija]

Looking great so far; thanks again for the PR!

I left a few comments in line; let me know what you think when you get a chance to respond.

As you mentioned, there's still tests and ref-guide docs missing here. Feel free to ping me when those are ready for review. (Or if you don't have time to get to them, lmk that too and maybe that's a place I can help!)

[gerlowskija]

[Q] This code is simple enough, but unless I'm missing something it seems like it could be replaced by the V2ApiUtils.squashIntoSolrResponse pattern we use on other APIs. Any particular reason you didn't go that route here?

[jdurham2843]

I used the approach above, because when I tried just squashing, the output I got back wouldn't fully serialize. I essentially would get this back:

	"responseHeader": {
		"status": 0,
		"QTime": 14
	},
	"snapshots": {
		"snapshot2": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@24e1513a",
		"snapshot1": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@50d291df",
		"snapshot9": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@55cf09d4",
		"snapshot8": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@542b33e3",
		"snapshot7": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@7a6dd0dd",
		"snapshot6": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@3ef5ea26",
		"snapshot5": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@13d9d585",
		"snapshot10": "org.apache.solr.core.snapshots.CollectionSnapshotMetaData:org.apache.solr.core.snapshots.CollectionSnapshotMetaData@1e604836"
	}
}

At the time, I couldn't figure out where to make changes to address the serialization problem, so I just sidestepped it. I'd be happy to take another stab at it though!

[gerlowskija]

Ah, ok. Actually this makes complete sense. The "squash" pattern only knows how to serialize particular types and interfaces, and I guess CollectionSnapshotMetaData isn't one of those. Forget I said anything, as long as the output looks correct 👍

[jdurham2843]

Gotcha, that makes sense. I went back and uncommented the for loop. CollectionSnapshotMetaData does have a toNamedList method, but its not an overriden method. Would it make sense at some point to create some kind of interface that exposes that method and then have it be used by serializers/MessageWriters? I think it would fall outside the scope of this change, but it could make sense if there were other classes out there that behaved the same way.

[gerlowskija]

[0] Minor typo, no closing paren around the v1 API endpoint.

(Also appears in the LIST and DELETE files

[jdurham2843]

Lazy copy-paste on my end, will address

(Did you leave off the closing paren on your last line above on purpose?) :)

[gerlowskija]

Yeah, a bit of a bad joke 😬 Glad you caught it though!

[gerlowskija]

[0] If you're up for it, this pattern (lines 89-103) happens enough in our "collection admin" APIs that there'd probably be some value in creating a method for it in AdminAPIBase.

[jdurham2843]

I like this idea, will do!

[gerlowskija]

[0] Might be worth extending AsyncJerseyResponse instead of 'SolrJerseyResponse' so that this field doesn't need repeated here.

[gerlowskija]

[0] There's definitely a tradeoff in raising the visibility of this method, but I've found it useful to make these "createRemoteMessage" implementations public static so that it can be more easily unit tested.

[gerlowskija]

[Q] Did you have a particular reason you made these values query-params, as opposed to values in a JSON request body or something?

I'm not against having these be query-params, but it does conflict a bit with the handful of other POST/PUT APIs that create some resource/entity in Solr. There's definitely a lot I don't know about API design, was curious if you had something particular in mind...

[jdurham2843]

I think receiving these as a JSON body makes sense. I believe I used QueryPararms, because I had used them in previous work, so didn't think too much. I do think JSON would be cleaner here, especially if it makes the API more consistent with other parts of the API.

[gerlowskija]

OK, sounds good. I agree with keeping it in the request body for now.

To think aloud a little bit here...

I've been mulling over for awhile now the idea of changing 'async' to be a query param instead of a property in the request-body. I think the intention in RESTful APIs is that the request-body of a PUT/POST is some representation of the "resource" being created. 'async' doesn't quite fit that conceptually - it's focused less on "what" resource is created and more on "how" the resource is created. But I'm still undecided personally, and it'll need changed in a few places even if we do decide to go that route. So definitely something to skip in terms of this PR.

[jdurham2843]

I think that makes a lot of sense, when the intent of the API is to create or update a resource. In that case it feels like things that affect "how" the work is done should be kept separate from the thing being acted on. Although, you could have a POST that falls more into the category of doing work within the "system" instead of just on a resource. Things that tweak that work could still come in via query-parameters, but a request-body may be more convenient. I guess in that scenario your "resource" is the "system"? In any case, I think either approach could be fine, as long as its consistent.

[jdurham2843]

Hi @gerlowskija - I believe I've addressed the most of the concerns listed above along with adding some unit tests that cover the Create and Delete collection snapshot endpoints. You mentioned in the jira ticket comments that the actual creation and deletion of snapshots was tested elsewhere, so I think just supplying unit tests may be all that's needed here. Spinning up a full cloud test case may be a little overkill! The only issue I haven't tackled yet is that V1 ListSnapshots serialization weirdness.

I'm still looking into the documentation portion of this work. It may be worth adding the API docs to collection-management.adoc like you mentioned, but I'll need to read through the backup-restore.adoc to see how I'd want to work them into there.

[gerlowskija]

New unit tests and other tweaks look great! We're just down to the docs at this point I think.

It may be worth adding the API docs to collection-management.adoc like you mentioned, but I'll need to read through the backup-restore.adoc to see how I'd want to work them into there.

Sounds good. Thinking on it a bit more, it might make sense to split those pieces up. Adding coverage to collection-management.adoc would be relatively rote small and rote, so it seems "in scope" for this PR. backup-restore.adoc probably needs some larger restructing, so I'd be fine creating a separate JIRA ticket for it and leaving it for another day. But that's just my 2c; happy to handle that however you'd like.

Jdurham2843 force-pushed the SOLR-15737-snapshot-collection-v2 branch from d461741 to 875d7a7
last week

One small nitpick process-wise: please avoid force-pushes if you can. It makes it slightly harder for others from adding small fixes/improvements to your branch, and it makes things a little harder to review in github by doing weird things to ongoing line-level review comments and nullifying Github's really nice "Diff files since your last review" feature.

[jdurham2843]

Collection-management.adoc has been updated with three sections on CREATESNAPSHOT, LISTSNAPSHOTS, and DELETESNAPSHOT. I think it's a good idea to do the backup-restore.adoc separately, since it wouldn't be nearly as simple to update and would need to consider other APIs aside from the three worked on here. Let me know what you think!

Regarding the force push: sorry about that! I needed to get some code in from the Apache Solr main branch, so I did a rebase and force push. That's normally the approach I reach for when needing to pull in changes, and can't do a fast-forward. Is there a another way to pull in changes, without needing to force push? My git-fu is fairly rudimentary

[gerlowskija]

Is there another way to pull in changes, without needing to force push?

Yep, definitely! IMO this stuff can be tricky to explain (or read about) through text alone. Everyone's git settings and repo tends to be just different enough that sharing workflow-commands alone can cause as much confusion as it clears up. Anyway, if this explanation doesn't help I'm happy to do a screenshare to discuss as well. (My email address should be on my GH profile)

But here goes!

---

Let's say I have a feature branch (my-feature) on a Github fork repo (myfork), and I want to pull in some changes from the main branch in the Apache repo (origin).

• The first step is to update your local copy of main so that it matches the latest stuff upstream:
  • git checkout main
  • `git pull origin main
• Now that your local copy of main is up to date, you can merge those changes into your feature branch
  • git checkout my-feature
  • git merge main
  • (Optionally fix any merge conflicts that might've arisen and git commit when you're happy)
• Your local my-feature branch now has the changes from main, we just need to publish your updated branch upstream:
  • git push myfork my-feature:my-feature

Hope that helps!

[gerlowskija]

LGTM, thanks again John for your work on this. 3 more APIs down!

I have the tests running now, but will otherwise aim to merge this shortly.

[gerlowskija]

[0] Just noticed the values being added to the response here. In general, I've been trying to change the response format as little as possible in these JAX-RS conversion PRs, and this is a bit of a departure from the existing v1 format.

That said, I think it's a change for the better and it's purely additive so it's not a backcompat concern. So let's go with it in this case.

[jdurham2843]

Awesome, and thank you for the review and help on this!

Regarding the git notes above: thanks for typing that out, I think it makes sense. I'll be sure to forgo the rebase/force-push approach on future PRs in favors of just doing merges. Thanks!

[alessandrobenedetti]

Is there any problem with the CHANGES.txt?
I see some broken checks here, and a contribution from a colleague of mine seems to have a broken crave build caused by the CHANGES.txt ?

Still investigating, so I am sorry if I said any nonsense!