[epugh]

Migrates org.apache.solr.pkg.PackageAPI from Solr's homegrown @EndPoint/@Command dispatch to standard JAX-RS annotations, consistent with other V2 APIs (ClusterFileStore, ZookeeperRead, ClusterProperty).

This has a pretty hefty update to the patterns, but I thought it was better to do it now while v2 api's are "experimental".

New REST endpoints

The command-based POST pattern is replaced with RESTful routes. Note the collection is now plural (/cluster/packages), matching the v2 API path conventions used elsewhere:

Old	New
GET /cluster/package	GET /cluster/packages
GET /cluster/package/{name}	GET /cluster/packages/{name} (now returns 404 if the package doesn't exist, instead of silently returning an empty result)
POST /cluster/package {"add": {"package":"p","version":"v","files":[...]}}	POST /cluster/packages/{name}/versions
POST /cluster/package {"delete": {"pkg":"p","version":"v"}}	DELETE /cluster/packages/{name}/versions/{version}
POST /cluster/package {"refresh": "name"}	POST /cluster/packages/{name}/refresh
(internal only, used by refresh for node-local reload)	POST /cluster/packages/{name}/refresh?localOnly=true
(internal only, used to make a node catch up to a ZK version)	POST /cluster/packages/sync?expectedVersion=N

The two internal-only rows replace an earlier version of this PR that smuggled the same inter-node signals onto GET /cluster/packages via refreshPackage/expectedVersion query params — a GET request shouldn't have side effects. localOnly mirrors the existing localDelete param on ClusterFileStoreApis.deleteFile.

Key structural changes

• PackageAPI — new JAX-RS implementation (extends JerseyResource implements PackageApis); replaces the old Edit/Read inner classes (~220 lines removed)
• PackageStore — renamed from the old PackageAPI; now clearly a ZK data/state holder (Packages, PkgVersion, ZK watcher), not an endpoint class. THIS WAS A BIG REFACTORING
• solr/api module — new PackageApis.java (JAX-RS interface), AddPackageVersionRequestBody.java, and two response types instead of one: ListPackagesResponse (the GET /cluster/packages collection response, map-keyed by package name) and GetPackageResponse (the GET /cluster/packages/{name} single-resource response, a flat object for just that package) — matching the List<Resource>Response/Get<Resource>Response naming and shape convention used by ListAliasesResponse/GetAliasByNameResponse and similar pairs elsewhere in the v2 API. OAS generation produces PackageApi SolrJ request classes automatically.
• PackageAPITest — integration test using SolrCloudTestCase (ZK required) with generated PackageApi SolrJ request classes
• TestPackages — updated to use new endpoints; error path changed from /details[0]/errorMessages[0] to /msg
• package-manager-internals.adoc — updated endpoint reference and curl examples
• dev-docs/apis-v2-conventions.adoc — added a new "Listing vs. Single-Resource Responses" section documenting the List<Resource>Response/Get<Resource>Response convention, since this PR is the first to need it spelled out explicitly

[gerlowskija]

Is there a JIRA ticket associated with this change @epugh?

[epugh]

Is there a JIRA ticket associated with this change @epugh?

No, because I viewed it as "just a refactoring and no one will notice....", and that I was hoping there were no debates that would require a JIRA to hash out!

[gerlowskija]

[0] Probably worth having a JIRA reference for this guy, even if it's just a tie-back to one of the v2 umbrella tickets? It's changing APIs and stuff, so worth tracking in more than just the PR description I'd imagine.

[epugh]

DOne

[gerlowskija]

[Q] Is there a reason you're not using the auto-generated SolrJ type here? We should have one now that the API is JAX-RS!

If this was an intentional decision made to prevent scope-creep, I'm fine with that. Just making sure it's not oversight.

[epugh]

so many places to update... I suspect in the future we need to do a "Check each GSR for fitness" check ;-)

[gerlowskija]

Like I said, I think not doing these bits in the same PR is a totally valid choice. I don't want to add extra steps that'd apply to all these PRs. But since the lines were changed in this PR already I wanted to check that it was an intentional choice is all 🤷

[epugh]

@gerlowskija I think I am ready for another review. I renamed PackageAPI to ClusterPackage to not conflict with PackageApis, but not married to that name! I am updating the spreadsheet.

[jaykay12]

@epugh I am planning to review this PR, can you make this one ready for review by rebasing with latest main?

[epugh]

@jaykay12 I added you as a collaborate to my epugh/solr... which I think lets you push to this repo? I hope this makes it easier as I have a couple of in progress ones you may wan tot pick up!

[epugh]

@jaykay12 would love your review! I think this is ready for merging.

[jaykay12]

Let me know if this minor plan change makes sense?

Ask is to just isolate the { renaming of PackageAPI -> PackageStore } in a follow-up PR.

[jaykay12]

this class is majorly (PackageAPI - {code no longer needed}), please correct me if i am wrong.

[suggestion] should we be splitting up PRs in those case, so that review stays easier & any unintentional change gets caught.

This PR could have been just pure migration & removal of deprecated stuff from PackageAPI class. In subsequent follow-up this renaming could have been attempted?

This is my pure thought, with this minor change, the PR size would have reduced by atleast 8-9 files.

[jaykay12]

should the core logic related to addVersion & deleteVersion be kept in PackageStore or PackageAPI for now (if earlier comment is chosen to be addressed)

that way, this class remains as thin adapter only.

[epugh]

Agreed, done in 7503001 -- moved the ZK atomicUpdate orchestration for both addVersion and deleteVersion into PackageStore. ClusterPackage now just validates/delegates/broadcasts:

Integer newVersion = packageStore.addVersion(packageName, requestBody);
if (newVersion != null) {
  notifyAllNodesToSync(newVersion);
}

addVersion() returns the new znode version (or null if the ZK update failed), preserving the original behavior of only notifying other nodes/refreshing local config when the update actually succeeded.

[jaykay12]

should this be kept as package private function inside PackageStore?

[reason]
this function mutates - packageStore.pkgs, ideally we can keep that ownership from within packageStore? if this function moves inside packageStore, we can esure package.pkgs is private to that class & no one changes its state from outside other than its own functions.

[epugh]

Agreed, moved in 7503001. syncToVersion is now a package-private instance method on PackageStore operating on its own pkgs field; ClusterPackage no longer assigns to packageStore.pkgs anywhere (confirmed -- grepped for it). This also makes it consistent with PackageStore's existing refreshPackages() watch handler, which already pairs a pkgs mutation with an immediate refreshPackageConf() call the same way.

[jaykay12]

this code comment still valid?

[jaykay12]

[info] why the key name as "result", why not just "packageInfo" here?

[jaykay12]

[question] why not "packages" as the key?

[jaykay12]

my local IntelliJ showed this warning.

please see, if applicable here.

[epugh]

thanks, going to look....

[epugh]

Confirmed via the compiler (-Xlint:deprecation):

AddPackageVersionRequestBody.java:27: warning: [deprecation] required() in Schema has been deprecated
AddPackageVersionRequestBody.java:33: warning: [deprecation] required() in Schema has been deprecated

@Schema(required = true) is deprecated in favor of @Schema(requiredMode = Schema.RequiredMode.REQUIRED). That said, this isn't something to fix just here -- 19 other files under solr/api/src/java/org/apache/solr/client/api/model/ use the exact same required = true attribute, so it's a pre-existing, codebase-wide pattern rather than something this PR introduced. Rather than fix it inconsistently in this one new file, I'll leave it as a follow-up to do as its own small cleanup PR across the whole module.

[epugh]

Per dev-docs/apis-v2-conventions.adoc's HTTP Methods section: "GET - used for non-mutating (i.e. 'read only') requests... Do not use for requests like queries that change state."

listPackages()/getPackage() are GET endpoints, but when called with expectedVersion they call syncToVersion(), which can reach this refreshPackageConf() call -- reloading plugin instances on this node. That's more than a pure read: it can swap out live plugin instances for concurrent requests being served on this node.

This is a defensible gray area (it's catching the node up to state already committed elsewhere in ZK, not originating a new mutation), but it's worth either a comment explaining the tradeoff, or considering whether the GET path should only re-read from ZK (without the full refreshPackageConf() reload) and leave the reload to the write paths / the explicit sync command.

[epugh]

Per dev-docs/apis-v2-conventions.adoc's "Unproxied APIs" section: paths like /api/node/key exist specifically to mark operations that only affect the receiving node and must never be proxied. syncPackages() fits that exactly -- it only ever touches the local PackageStore (see ClusterPackage.syncToVersion), and is invoked exclusively via broadcastToLiveNodes()/processWithBaseUrl() targeting one specific node's base URL. The @Operation summary here says as much: "Used internally for inter-node fan-out after a package change; not normally invoked directly."

Recommend POST /api/node/packages/sync instead of /cluster/packages/sync, following the existing /node/{resource} pattern (NodeSystemInfoApi → /node/system, NodeHealthApi → /node/health, and especially GetNodeCommandStatusApi → /node/commands/, the closest precedent as another receiving-node-only, not-cluster-tracked concept) combined with the "command API" convention already used elsewhere in this PR (refreshPackage → .../refresh): packages as the resource, sync as the appended command verb. Only this one endpoint would need to move -- listPackages/getPackage are genuinely cluster-wide, proxy-friendly reads and should stay under /cluster/packages.

[epugh]

Hey @jaykay12 your feedback was super valuable!!!!! I rethought things based on your input!