-
Notifications
You must be signed in to change notification settings - Fork 808
Separate core specific Request Writers from node specific "built in" ones. Move core specific to using ImplicitPlugins.json. #4073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Admin/container-level requests (e.g., /admin/cores, /admin/collections) have no associated SolrCore and cannot use the core's response writer registry loaded from ImplicitPlugins.json. This commit introduces ADMIN_RESPONSE_WRITERS, a minimal set of 6 response writers specifically for admin requests. ADMIN_RESPONSE_WRITERS contains only the formats actually needed by admin APIs: - javabin: Required by SolrJ clients (default programmatic API) - json: Required by Admin UI (web interface) - xml: Backward compatibility and occasional test usage - prometheus/openmetrics: Required by /admin/metrics endpoint - standard: Alias for json Benefits: - Clearer separation between admin vs core response writers - Smaller footprint (6 entries vs 14 in DEFAULT_RESPONSE_WRITERS) - Better documentation of admin API requirements - No breaking changes to existing functionality Core-specific requests continue to use the full ImplicitPlugins.json system with ConfigOverlay support. DEFAULT_RESPONSE_WRITERS is marked @deprecated but remains available for backward compatibility. Changes: - SolrCore: Add ADMIN_RESPONSE_WRITERS map and getAdminResponseWriter() method - SolrCore: Mark DEFAULT_RESPONSE_WRITERS as @deprecated with updated Javadoc - SolrQueryRequest: Use getAdminResponseWriter() when core is null - HttpSolrCall: Use getAdminResponseWriter() for admin request handling - TestImplicitPlugins: Add tests for admin writers and backward compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR moves core-specific QueryResponseWriter defaults out of hardcoded Java and into ImplicitPlugins.json, while introducing a minimal built-in response writer set for admin/container-level requests that have no SolrCore.
Changes:
- Add
BuiltInResponseWriterRegistryand switch admin/no-core writer selection to use it. - Load core response writers from
ImplicitPlugins.jsonduringSolrCoreinitialization. - Add tests and a changelog entry documenting the new separation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| solr/core/src/java/org/apache/solr/response/BuiltInResponseWriterRegistry.java | New minimal built-in writer registry for no-core/admin requests |
| solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java | Admin request writer selection now uses the built-in registry |
| solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java | Default wt writer selection now falls back to built-in writers when core is null |
| solr/core/src/java/org/apache/solr/core/SolrCore.java | Core writer initialization now loads from ImplicitPlugins.json; implicit plugin loading generalized |
| solr/core/src/resources/ImplicitPlugins.json | Adds queryResponseWriter section for core-configurable response writers |
| solr/core/src/test/org/apache/solr/response/TestBuiltInResponseWriterRegistry.java | New tests for built-in registry fallback/availability |
| solr/core/src/test/org/apache/solr/core/TestImplicitPlugins.java | New tests asserting core vs built-in writer separation |
| solr/benchmark/src/java/org/apache/solr/bench/search/QueryResponseWriters.java | Removes stale reference to SolrCore#DEFAULT_RESPONSE_WRITERS |
| changelog/unreleased/admin-response-writers-minimal-set.yml | Documents behavior change for admin/container response writers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| info.className, | ||
| QueryResponseWriter.class, | ||
| "queryResponseWriter", | ||
| null, |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SolrCore.initWriters() passes core=null into createInstance(...) when loading core-level response writers. If any QueryResponseWriter has a SolrCore constructor (or otherwise expects a non-null core), this will either instantiate with a null core or skip the core-aware constructor path. Use this (the current SolrCore) instead of null when creating core-specific writers.
| null, | |
| this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting, and I think a separte issue. None of the QueryResponseWriter's take a core. RawResponseWriter looks up a core from a specific request, but doesn't take it directly. There are. a few calls to the createInstance and they mostly pass a null value. The only place I see a core being passed in is the call from PackagePluginHolder.initNewInstance. I don't know if there is a refactoring to have two createInstances? Or one that you call that doesn't take a core and then calls this one for a null? Probably for another PR. So, based on this analysis, I am going to leave this null. This is just a bit icky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to have a version of createInstance that doesn't take a SolrCore just to make it clearer and use that in the places we call createInstance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a change -- were response writers receiving the core and now, not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was NOT a change. Only in one instance (that of PackagePluginHolder) do they get a core. I think it's a refactoring opportunity.
| } catch (Exception e) { | ||
| log.warn("Failed to load implicit response writer: {}", info.name, e); | ||
| } | ||
| } | ||
|
|
||
| // Add special filestream writer (custom implementation) | ||
| defaultWriters.put(ReplicationAPIBase.FILE_STREAM, getFileStreamWriter()); | ||
|
|
||
| // Initialize with the built defaults | ||
| responseWriters.init(defaultWriters, this); | ||
|
|
||
| // configure the default response writer; this one should never be null | ||
| if (responseWriters.getDefault() == null) responseWriters.setDefault("standard"); | ||
| } |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SolrCore.initWriters() swallows exceptions when loading implicit response writers (logs warn and continues). If the "standard" writer (or any required default) fails to load, responseWriters may end up with no default (PluginBag#setDefault is a no-op when the name is missing), violating the “Never null” contract of getQueryResponseWriter(..) and potentially causing NPEs later. Consider failing fast when required writers can’t be loaded, or explicitly ensure a default writer is present (e.g., fall back to a built-in JSON writer) before continuing startup.
| title: Introduce minimal ADMIN_RESPONSE_WRITERS for admin/container-level requests. Admin requests now use a minimal set of 6 response writers (javabin, json, xml, prometheus, openmetrics, standard) instead of the full 14-writer DEFAULT_RESPONSE_WRITERS map. Core-specific requests continue to use ImplicitPlugins.json with ConfigOverlay support. | ||
| type: changed | ||
| authors: | ||
| - name: Eric Pugh | ||
| links: | ||
| - name: GitHub Discussion | ||
| url: https://github.com/apache/solr/discussions | ||
| notes: | | ||
| Admin/container-level requests (e.g., /admin/cores, /admin/collections) have no associated SolrCore | ||
| and cannot use the core's response writer registry loaded from ImplicitPlugins.json. Previously, | ||
| these requests used DEFAULT_RESPONSE_WRITERS (14 entries). Now they use ADMIN_RESPONSE_WRITERS (6 entries) | ||
| containing only the formats actually needed by admin APIs: | ||
| - javabin (SolrJ clients) | ||
| - json (Admin UI) | ||
| - xml (backward compatibility) | ||
| - prometheus/openmetrics (metrics endpoint) | ||
| - standard (alias for json) | ||
| This provides clearer separation of concerns and better documentation of requirements. Core-specific | ||
| requests are unaffected and continue to use the full ImplicitPlugins.json system with ConfigOverlay support. | ||
| DEFAULT_RESPONSE_WRITERS remains available but is now deprecated for internal use. No newline at end of file |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog entry refers to an "ADMIN_RESPONSE_WRITERS" name and says "DEFAULT_RESPONSE_WRITERS remains available". In this PR, DEFAULT_RESPONSE_WRITERS is removed from SolrCore and replaced by BuiltInResponseWriterRegistry (and a deprecated SolrCore.getAdminResponseWriter method). Please update this changelog text to match the actual public/internal API surface introduced by the PR so release notes aren’t misleading.
| QueryResponseWriter respWriter = | ||
| SolrCore.DEFAULT_RESPONSE_WRITERS.get(solrReq.getParams().get(CommonParams.WT)); | ||
| BuiltInResponseWriterRegistry.getWriter(solrReq.getParams().get(CommonParams.WT)); | ||
| if (respWriter == null) respWriter = getResponseWriter(); |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpSolrCall.logAndFlushAdminRequest() still checks if (respWriter == null) after calling BuiltInResponseWriterRegistry.getWriter(). That method currently never returns null (it always falls back to "standard"), so this branch is dead code and can be removed (or change getWriter() contract if null is actually expected).
| if (respWriter == null) respWriter = getResponseWriter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a fair comment. It's that werid "standard" thing. We should either solve the null or delete this.
| QueryResponseWriter.class, | ||
| "queryResponseWriter", | ||
| null, | ||
| getResourceLoader()); |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SolrCore.initWriters() instantiates implicit response writers via createInstance(...) but does not run plugin initialization (init args / PluginInfoInitialized / NamedListInitializedPlugin) like other Solr plugins do (see SolrCore.createInitInstance / SolrCore.initPlugin). This can break response writers that expect initialization from PluginInfo. After instantiating, call SolrCore.initPlugin(info, writer) (or use createInitInstance with a PluginInfo) before registering it.
| getResourceLoader()); | |
| getResourceLoader()); | |
| initPlugin(info, writer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably a "good thing" yet none of our QueryResponseWriters need it, so we don't have it. If this plugin creation was centralized more, then it might happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feedback from Copilot.
Agreed that we should better normalize/standardize plugin instantiation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the VelocityResponseWriter needs a SolrResourceLoader (from a core). Any way, we need not concern ourselves with that.
The **`BuiltInResponseWriters`** name would be the most consistent with existing Solr patterns, especially since `RequestHandlers` serves a very similar purpose for request handlers that your class serves for response writers.
| solrResp.getToLogAsString("[admin]")); | ||
| } | ||
| } | ||
| // Admin requests have no core, use built-in writers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be calling these "Cluster requests" or "Builtin requests"? Admin isn't really a very precise term since we would call "Add a shard" a "admin request"....? Or make sure that when we say "Admin" we mean "Solr Cluster Administration"???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"node/container"
|
@dsmiley I think I am ready for some review based on the conversation on the dev mailing list. I'm going to go through and self review now, which might be useful to highlight points of discussion! |
| @@ -0,0 +1,22 @@ | |||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | |||
| title: Introduce minimal ADMIN_RESPONSE_WRITERS for admin/container-level requests. Admin requests now use a minimal set of 6 response writers (javabin, json, xml, prometheus, openmetrics, standard) instead of the full 14-writer DEFAULT_RESPONSE_WRITERS map. Core-specific requests continue to use ImplicitPlugins.json with ConfigOverlay support. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to redo this... Claude generated (and committed! bad claude!) this...
| } | ||
|
|
||
| // Add special filestream writer (custom implementation) | ||
| defaultWriters.put(ReplicationAPIBase.FILE_STREAM, getFileStreamWriter()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I don't know why this is "special". I looked, and I think we could just create a class called FileStreamWriter instead of this special magic. Claude agreed with me and even cranked out a class. I think that would simplify the code flow. Maybe we call it REPLICATION_FILE_STREAM or REPLICATION_STREAM. We don't have to fix in this, but it's a candiate for an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you looked into this. Seems in-scope to include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great... Going to roll that into this PR.
|
|
||
| /** The writer to use for this request, considering {@link CommonParams#WT}. Never null. */ | ||
| /** | ||
| * The writer to use for this request, considering {@link CommonParams#WT}. Never null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is saying that the wt is always there, which means we have some extra "if no wt, than do this" which wouldn't match these docs?
| * | ||
| * <p>If a core is available, uses the core's response writer registry (loaded from | ||
| * ImplicitPlugins.json with ConfigOverlay support). If no core is available (e.g., for | ||
| * admin/container requests), uses a minimal set of admin-appropriate writers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
admin thing again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe in javadocs over-specifying implementation details (like added here). Let us be free to change the implementation details without maintaining these docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i pruned it back some... all the javadocs in this class are fairly extensive...
| Map<String, QueryResponseWriter> builtinWriters = new HashMap<>(6, 1); | ||
| builtinWriters.put(CommonParams.JAVABIN, new JavaBinResponseWriter()); | ||
| builtinWriters.put(CommonParams.JSON, new JacksonJsonWriter()); | ||
| builtinWriters.put("standard", builtinWriters.get(CommonParams.JSON)); // Alias for JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this term "standard" is just a weird indirection. Why can't we just say "json" everywhere. We know that the way Solr talks is JSON. and if it isn't, then we specify the format like JAVABIN. I don't get what the term "standard" gives us, and it's just another thing to know about. I'd love to delete it and just use the word "json". There is no "better" or "compacter" or any other type to contrast with "standard". Rant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate issue, but I propose we universally remove the alias "standard" from all plugin types. It's such a Solr old-school thing that I don't see anyone use. Solr 11 only, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://issues.apache.org/jira/browse/SOLR-18085 for this. I will remove my ranting about this from this PR now that we have a plan and a JIRA.
| * @return the response writer, never null (returns "standard"/JSON if not found) | ||
| */ | ||
| public static QueryResponseWriter getWriter(String writerName) { | ||
| // carrying over this "standard" thing from original code, but do we want this? null/blank |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More ranting! This time in the code comments.
dsmiley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(partial review; barely did anything. I'll continue this evening)
| info.className, | ||
| QueryResponseWriter.class, | ||
| "queryResponseWriter", | ||
| null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a change -- were response writers receiving the core and now, not?
| QueryResponseWriter.class, | ||
| "queryResponseWriter", | ||
| null, | ||
| getResourceLoader()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feedback from Copilot.
Agreed that we should better normalize/standardize plugin instantiation!
| } | ||
|
|
||
| // Add special filestream writer (custom implementation) | ||
| defaultWriters.put(ReplicationAPIBase.FILE_STREAM, getFileStreamWriter()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you looked into this. Seems in-scope to include.
| QueryResponseWriter.class, | ||
| "queryResponseWriter", | ||
| null, | ||
| getResourceLoader()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the VelocityResponseWriter needs a SolrResourceLoader (from a core). Any way, we need not concern ourselves with that.
| * | ||
| * <p>If a core is available, uses the core's response writer registry (loaded from | ||
| * ImplicitPlugins.json with ConfigOverlay support). If no core is available (e.g., for | ||
| * admin/container requests), uses a minimal set of admin-appropriate writers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe in javadocs over-specifying implementation details (like added here). Let us be free to change the implementation details without maintaining these docs.
| /** | ||
| * Registry for built-in response writers in Solr. | ||
| * | ||
| * <p>Manages a minimal set of essential response writers that are always available, regardless of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these javadocs are over specified. IMO it shouldn't speak of cores or ImplicitPlugins. But @see links to key methods (that might be in SolrCore) is very helpful.
I've noticed LLMs haven't been good, by and large, on link-ifying documentation. It just puts text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... revamping.
| * csv, geojson, graphml, smile, etc.), use the SolrCore's response writer registry which is loaded | ||
| * from ImplicitPlugins.json and supports ConfigOverlay customizations. | ||
| */ | ||
| public class BuiltInResponseWriters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about calling this ResponseWriterRegistry? Or ResponseWriters Presently, just "built-in" stuff but I could see one day increasing/changing to hold node level plugins that may be registered in advance via solr.xml or ServiceLoader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny, I started with BuiltInResponseWritersRegistery and then feeling like the suffic Registry was odd... We don't use that word anywhere else. I suppose ResponseWriters, but then you might think that it should have the core level ones as well? Yeah, this one is hard. Maybe adding Registry is good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Registry" is the perfect suffix, and we should use it no matter how rare it might be in the codebase. People should grok this work immediately. I think we tend to use the word Factory but that is suggestive of more construction/management then merely holding them.
solr/core/src/java/org/apache/solr/response/BuiltInResponseWriters.java
Outdated
Show resolved
Hide resolved
| Map<String, QueryResponseWriter> builtinWriters = new HashMap<>(6, 1); | ||
| builtinWriters.put(CommonParams.JAVABIN, new JavaBinResponseWriter()); | ||
| builtinWriters.put(CommonParams.JSON, new JacksonJsonWriter()); | ||
| builtinWriters.put("standard", builtinWriters.get(CommonParams.JSON)); // Alias for JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate issue, but I propose we universally remove the alias "standard" from all plugin types. It's such a Solr old-school thing that I don't see anyone use. Solr 11 only, of course.
| solrResp.getToLogAsString("[admin]")); | ||
| } | ||
| } | ||
| // Admin requests have no core, use built-in writers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"node/container"
…ResponseWriter Make the class an explicit class, and add a test. This removes some extra code from SolrCore.
|
Okay, aside from the naming of the |
dsmiley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks very close to mergeable
| // Prevent instantiation | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly, this field needs no javadocs at all. It's private and nobody is going to read these javadocs to learn about Solr's response writers. If you feel a modicum of javadocs are needed on each one, then add to the javadocs of these classes (which probably already have docs), not here.
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | ||
| title: Introduce minimal ADMIN_RESPONSE_WRITERS for admin/container-level requests. Admin requests now use a minimal set of 6 response writers (javabin, json, xml, prometheus, openmetrics, standard) instead of the full 14-writer DEFAULT_RESPONSE_WRITERS map. Core-specific requests continue to use ImplicitPlugins.json with ConfigOverlay support. | ||
| title: Introduce minimal set of request writers for node/container-level requests. Core-specific request writers now leverage ImplicitPlugins.json for creation. | ||
| type: changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no; "other" since it's a refactoring.
https://issues.apache.org/jira/browse/SOLR-XXXXX
Description
RequestWriters today are hardcoded map that is used by everyone in a Node. However, there are a set of RequestWriters that are required at the Node level, and then a bunch of optional ones that are specific to an individual core/collection.
Secondly, we have an existing mechanism based on
ImplicitPlugins.jsonfile used to configure core/collection plugins that can easily be extended to support our RequestWriters.Solution
Moves Core specific ResponseWriters to being configured in
ImplicitPlugins.json. Moves the node required ResponseWriters used by the rest of Solr in a neworg.apache.solr.response.BuiltInResponseWriterRegistryclass.Tests
Existing tests, need to do more real world..