dsmiley commented on code in PR #4073:
URL: https://github.com/apache/solr/pull/4073#discussion_r2724344977
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3148,11 +3117,48 @@ default String getContentType() {
}
/**
- * Configure the query response writers. There will always be a default
writer; additional writers
- * may also be configured.
+ * Gets a response writer suitable for admin/container-level requests.
Review Comment:
+1 to "node/container" level (sadly both terms; container is too pervasive).
I'd also like us to avoid using the word "admin" as a synonym for that, which
is too ambiguous/conflated of a word.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3148,11 +3116,49 @@ default String getContentType() {
}
/**
- * Configure the query response writers. There will always be a default
writer; additional writers
- * may also be configured.
+ * Gets a response writer suitable for admin/container-level requests.
+ *
+ * @param writerName the writer name, or null for default
+ * @return the response writer, never null
+ * @deprecated Use {@link
+ *
org.apache.solr.response.BuiltInResponseWriterRegistry#getWriter(String)}
instead.
+ */
+ @Deprecated
+ public static QueryResponseWriter getAdminResponseWriter(String writerName) {
+ return
org.apache.solr.response.BuiltInResponseWriterRegistry.getWriter(writerName);
+ }
+
+ /**
+ * Initializes query response writers. Response writers from {@code
ImplicitPlugins.json} may also
+ * be configured.
*/
private void initWriters() {
- responseWriters.init(DEFAULT_RESPONSE_WRITERS, this);
+ // Build default writers map from implicit plugins
+ Map<String, QueryResponseWriter> defaultWriters = new HashMap<>();
+
+ // Load writers from ImplicitPlugins.json
+ List<PluginInfo> implicitWriters = getImplicitResponseWriters();
+ for (PluginInfo info : implicitWriters) {
+ try {
+ QueryResponseWriter writer =
+ createInstance(
+ info.className,
+ QueryResponseWriter.class,
+ "queryResponseWriter",
+ null,
Review Comment:
is this a change -- were response writers receiving the core and now, not?
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3148,11 +3116,49 @@ default String getContentType() {
}
/**
- * Configure the query response writers. There will always be a default
writer; additional writers
- * may also be configured.
+ * Gets a response writer suitable for admin/container-level requests.
+ *
+ * @param writerName the writer name, or null for default
+ * @return the response writer, never null
+ * @deprecated Use {@link
+ *
org.apache.solr.response.BuiltInResponseWriterRegistry#getWriter(String)}
instead.
+ */
+ @Deprecated
+ public static QueryResponseWriter getAdminResponseWriter(String writerName) {
+ return
org.apache.solr.response.BuiltInResponseWriterRegistry.getWriter(writerName);
+ }
+
+ /**
+ * Initializes query response writers. Response writers from {@code
ImplicitPlugins.json} may also
+ * be configured.
*/
private void initWriters() {
- responseWriters.init(DEFAULT_RESPONSE_WRITERS, this);
+ // Build default writers map from implicit plugins
+ Map<String, QueryResponseWriter> defaultWriters = new HashMap<>();
+
+ // Load writers from ImplicitPlugins.json
+ List<PluginInfo> implicitWriters = getImplicitResponseWriters();
+ for (PluginInfo info : implicitWriters) {
+ try {
+ QueryResponseWriter writer =
+ createInstance(
+ info.className,
+ QueryResponseWriter.class,
+ "queryResponseWriter",
+ null,
+ getResourceLoader());
Review Comment:
Great feedback from Copilot.
Agreed that we should better normalize/standardize plugin instantiation!
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3148,11 +3117,48 @@ default String getContentType() {
}
/**
- * Configure the query response writers. There will always be a default
writer; additional writers
- * may also be configured.
+ * Gets a response writer suitable for admin/container-level requests.
+ *
+ * @param writerName the writer name, or null for default
+ * @return the response writer, never null
+ * @deprecated Use {@link BuiltInResponseWriters#getWriter(String)} instead.
+ */
+ @Deprecated
+ public static QueryResponseWriter getAdminResponseWriter(String writerName) {
+ return BuiltInResponseWriters.getWriter(writerName);
+ }
+
+ /**
+ * Initializes query response writers. Response writers from {@code
ImplicitPlugins.json} may also
+ * be configured.
*/
private void initWriters() {
- responseWriters.init(DEFAULT_RESPONSE_WRITERS, this);
+ // Build default writers map from implicit plugins
+ Map<String, QueryResponseWriter> defaultWriters = new HashMap<>();
+
+ // Load writers from ImplicitPlugins.json
+ List<PluginInfo> implicitWriters = getImplicitResponseWriters();
+ for (PluginInfo info : implicitWriters) {
+ try {
+ QueryResponseWriter writer =
+ createInstance(
+ info.className,
+ QueryResponseWriter.class,
+ "queryResponseWriter",
+ null,
+ getResourceLoader());
+ defaultWriters.put(info.name, writer);
+ } 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());
Review Comment:
Glad you looked into this. Seems in-scope to include.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
