Separate core specific Request Writers from node specific "built in" ones. Move core specific to using ImplicitPlugins.json. #4073
+570
−89
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | ||
| title: Introduce minimal set of request writers for node/container-level requests. Core-specific request writers now leverage ImplicitPlugins.json for creation. | ||
| type: other | ||
| authors: | ||
| - name: Eric Pugh | ||
| - name: David Smiley | ||
| links: | ||
| - name: PR#4073 | ||
| url: https://github.com/apache/solr/pull/4073 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,8 +17,6 @@ | |||||||
| package org.apache.solr.core; | ||||||||
|
|
||||||||
| import static org.apache.solr.common.params.CommonParams.PATH; | ||||||||
| import static org.apache.solr.metrics.SolrCoreMetricManager.COLLECTION_ATTR; | ||||||||
| import static org.apache.solr.metrics.SolrCoreMetricManager.CORE_ATTR; | ||||||||
| import static org.apache.solr.metrics.SolrCoreMetricManager.REPLICA_TYPE_ATTR; | ||||||||
|
|
@@ -112,7 +110,6 @@ | |||||||
| import org.apache.solr.handler.IndexFetcher; | ||||||||
| import org.apache.solr.handler.RequestHandlerBase; | ||||||||
| import org.apache.solr.handler.SolrConfigHandler; | ||||||||
| import org.apache.solr.handler.api.V2ApiUtils; | ||||||||
| import org.apache.solr.handler.component.HighlightComponent; | ||||||||
| import org.apache.solr.handler.component.SearchComponent; | ||||||||
|
|
@@ -129,19 +126,9 @@ | |||||||
| import org.apache.solr.request.SolrQueryRequest; | ||||||||
| import org.apache.solr.request.SolrRequestHandler; | ||||||||
| import org.apache.solr.request.SolrRequestInfo; | ||||||||
| import org.apache.solr.response.QueryResponseWriter; | ||||||||
| import org.apache.solr.response.ResponseWritersRegistry; | ||||||||
| import org.apache.solr.response.SolrQueryResponse; | ||||||||
| import org.apache.solr.response.transform.TransformerFactory; | ||||||||
| import org.apache.solr.rest.ManagedResourceStorage; | ||||||||
| import org.apache.solr.rest.ManagedResourceStorage.StorageIO; | ||||||||
|
|
@@ -3088,51 +3075,6 @@ public PluginBag<QueryResponseWriter> getResponseWriters() { | |||||||
|
|
||||||||
| private final PluginBag<QueryResponseWriter> responseWriters = | ||||||||
| new PluginBag<>(QueryResponseWriter.class, this); | ||||||||
|
|
||||||||
| public void fetchLatestSchema() { | ||||||||
| IndexSchema schema = configSet.getIndexSchema(true); | ||||||||
|
|
@@ -3148,11 +3090,48 @@ default String getContentType() { | |||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Gets a response writer suitable for node/container-level requests. | ||||||||
| * | ||||||||
| * @param writerName the writer name, or null for default | ||||||||
| * @return the response writer, never null | ||||||||
| * @deprecated Use {@link ResponseWritersRegistry#getWriter(String)} instead. | ||||||||
| */ | ||||||||
| @Deprecated | ||||||||
| public static QueryResponseWriter getAdminResponseWriter(String writerName) { | ||||||||
| return ResponseWritersRegistry.getWriter(writerName); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Initializes query response writers. Response writers from {@code ImplicitPlugins.json} may also | ||||||||
| * be configured. | ||||||||
| */ | ||||||||
| private void initWriters() { | ||||||||
| // Build default writers map from implicit plugins | ||||||||
| Map<String, QueryResponseWriter> defaultWriters = new HashMap<>(); | ||||||||
|
|
||||||||
| // Start with built-in writers that are always available | ||||||||
| defaultWriters.putAll(ResponseWritersRegistry.getAllWriters()); | ||||||||
|
|
||||||||
| // Load writers from ImplicitPlugins.json (may override built-ins) | ||||||||
| List<PluginInfo> implicitWriters = getImplicitResponseWriters(); | ||||||||
| for (PluginInfo info : implicitWriters) { | ||||||||
| try { | ||||||||
| QueryResponseWriter writer = | ||||||||
| createInstance( | ||||||||
| info.className, | ||||||||
| QueryResponseWriter.class, | ||||||||
| "queryResponseWriter", | ||||||||
| null, | ||||||||
| getResourceLoader()); | ||||||||
|
There was a problem hiding this comment. 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.
Suggested change
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. Great feedback from Copilot.
Contributor
There was a problem hiding this comment. I suspect the VelocityResponseWriter needs a SolrResourceLoader (from a core). Any way, we need not concern ourselves with that. |
||||||||
| defaultWriters.put(info.name, writer); | ||||||||
| } catch (Exception e) { | ||||||||
| log.warn("Failed to load implicit response writer: {}", info.name, e); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // 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"); | ||||||||
| } | ||||||||
|
|
@@ -3614,32 +3593,49 @@ public void cleanupOldIndexDirectories(boolean reload) { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| private static final class ImplicitPluginsHolder { | ||||||||
| private ImplicitPluginsHolder() {} | ||||||||
|
|
||||||||
| private static final Map<String, List<PluginInfo>> ALL_IMPLICIT_PLUGINS; | ||||||||
|
|
||||||||
| static { | ||||||||
| @SuppressWarnings("unchecked") | ||||||||
| Map<String, ?> implicitPluginsInfo = | ||||||||
| (Map<String, ?>) | ||||||||
| Utils.fromJSONResource(SolrCore.class.getClassLoader(), "ImplicitPlugins.json"); | ||||||||
|
|
||||||||
| Map<String, List<PluginInfo>> plugins = new HashMap<>(); | ||||||||
|
|
||||||||
| // Load all plugin types from the JSON | ||||||||
| for (Map.Entry<String, ?> entry : implicitPluginsInfo.entrySet()) { | ||||||||
| String pluginType = entry.getKey(); | ||||||||
| @SuppressWarnings("unchecked") | ||||||||
| Map<String, Map<String, Object>> pluginConfigs = | ||||||||
| (Map<String, Map<String, Object>>) entry.getValue(); | ||||||||
|
|
||||||||
| List<PluginInfo> pluginInfos = new ArrayList<>(pluginConfigs.size()); | ||||||||
| for (Map.Entry<String, Map<String, Object>> plugin : pluginConfigs.entrySet()) { | ||||||||
| Map<String, Object> info = plugin.getValue(); | ||||||||
| info.put(CommonParams.NAME, plugin.getKey()); | ||||||||
| pluginInfos.add(new PluginInfo(pluginType, info)); | ||||||||
| } | ||||||||
| plugins.put(pluginType, Collections.unmodifiableList(pluginInfos)); | ||||||||
| } | ||||||||
|
|
||||||||
| ALL_IMPLICIT_PLUGINS = Collections.unmodifiableMap(plugins); | ||||||||
| } | ||||||||
|
|
||||||||
| public static List<PluginInfo> getImplicitPlugins(String type) { | ||||||||
| return ALL_IMPLICIT_PLUGINS.getOrDefault(type, Collections.emptyList()); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| public List<PluginInfo> getImplicitHandlers() { | ||||||||
| return ImplicitPluginsHolder.getImplicitPlugins(SolrRequestHandler.TYPE); | ||||||||
| } | ||||||||
|
|
||||||||
| public List<PluginInfo> getImplicitResponseWriters() { | ||||||||
| return ImplicitPluginsHolder.getImplicitPlugins("queryResponseWriter"); | ||||||||
| } | ||||||||
|
|
||||||||
| public CancellableQueryTracker getCancellableQueryTracker() { | ||||||||
|
|
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.solr.response; | ||
|
|
||
| import java.io.Closeable; | ||
| import java.io.IOException; | ||
| import java.io.OutputStream; | ||
| import org.apache.solr.client.solrj.response.JavaBinResponseParser; | ||
| import org.apache.solr.core.SolrCore; | ||
| import org.apache.solr.handler.admin.api.ReplicationAPIBase; | ||
| import org.apache.solr.request.SolrQueryRequest; | ||
|
|
||
| /** | ||
| * Response writer for file streaming operations, used for replication, exports, and other core Solr | ||
| * operations. | ||
| * | ||
| * <p>This writer handles streaming of large files (such as index files) by looking for a {@link | ||
| * org.apache.solr.core.SolrCore.RawWriter} object in the response under the {@link | ||
| * ReplicationAPIBase#FILE_STREAM} key. When found, it delegates directly to the raw writer to | ||
| * stream the file content efficiently. | ||
| * | ||
| * <p>This writer is specifically designed for replication file transfers and provides no fallback | ||
| * behavior - it only works when a proper RawWriter is present in the response. | ||
| */ | ||
| public class FileStreamResponseWriter implements QueryResponseWriter { | ||
|
|
||
| @Override | ||
| public void write( | ||
| OutputStream out, SolrQueryRequest request, SolrQueryResponse response, String contentType) | ||
| throws IOException { | ||
| SolrCore.RawWriter rawWriter = | ||
| (SolrCore.RawWriter) response.getValues().get(ReplicationAPIBase.FILE_STREAM); | ||
| if (rawWriter != null) { | ||
| rawWriter.write(out); | ||
| if (rawWriter instanceof Closeable closeable) { | ||
| closeable.close(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getContentType(SolrQueryRequest request, SolrQueryResponse response) { | ||
| SolrCore.RawWriter rawWriter = | ||
| (SolrCore.RawWriter) response.getValues().get(ReplicationAPIBase.FILE_STREAM); | ||
| if (rawWriter != null) { | ||
| String contentType = rawWriter.getContentType(); | ||
| if (contentType != null) { | ||
| return contentType; | ||
| } | ||
| } | ||
| return JavaBinResponseParser.JAVABIN_CONTENT_TYPE; | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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=nullinto 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. Usethis(the current SolrCore) instead of null when creating core-specific 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.
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
nullvalue. The only place I see a core being passed in is the call fromPackagePluginHolder.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
createInstancethat doesn't take a SolrCore just to make it clearer and use that in the places we callcreateInstanceThere 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.