dsmiley commented on code in PR #975:
URL: https://github.com/apache/solr/pull/975#discussion_r965317709
##########
solr/core/src/java/org/apache/solr/handler/admin/api/SchemaNameAPI.java:
##########
@@ -46,7 +46,7 @@ public SchemaNameAPI(SolrCore solrCore) {
}
@GET
- @Produces("application/json")
+ @Produces({"application/json", "application/xml", "application/javabin"})
Review Comment:
(pending @gerlowskija )
##########
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##########
@@ -273,7 +278,7 @@ public static Api getApiInfo(
}
if (api == null) {
- return getSubPathApi(requestHandlers, path, fullPath, new
CompositeApi(null));
Review Comment:
I agree we should jettison introspect for OpenAPI integration
##########
solr/core/src/java/org/apache/solr/jersey/RequestContextConstants.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.jersey;
+
+import com.codahale.metrics.Timer;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.ws.rs.container.ContainerRequestContext;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.handler.RequestHandlerBase;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.security.AuthorizationContext;
+
+/**
+ * Keys used to store and retrieve values from the Jersey request context.
+ *
+ * <p>Properties are generally set in V2HttpCall's 'invokeJerseyRequest' and
retrieved in individual
+ * {@link javax.ws.rs.container.ContainerRequestFilter}s using {@link
+ * ContainerRequestContext#getProperty(String)}
+ */
+public class RequestContextConstants {
Review Comment:
No, but such a strange act would nonetheless be harmless.
##########
solr/core/src/java/org/apache/solr/jersey/SolrJerseyResponse.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.jersey;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/**
+ * Base response-body POJO to be used by Jersey resources.
+ *
+ * <p>Contains fields common to all Solr API responses, particularly the
'responseHeader' and
+ * 'error' fields.
+ */
+public class SolrJerseyResponse implements JacksonReflectMapWriter {
+
+ @JsonProperty("responseHeader")
+ public ResponseHeader responseHeader = new ResponseHeader();
+
+ @JsonProperty("error")
+ public ErrorInfo error;
+
+ public static class ResponseHeader implements JacksonReflectMapWriter {
Review Comment:
Rigidity. I've seen and probably written plugins that add to the header as
a map of whatever info you want from, say, a SearchComponent. Will JAX-RS
prevent such plugins from doing so?
##########
solr/core/src/java/org/apache/solr/core/PluginBag.java:
##########
@@ -213,6 +237,24 @@ public PluginHolder<T> put(String name, PluginHolder<T>
plugin) {
apiBag.register(api, nameSubstitutes);
}
}
+
+ // TODO Should we use <requestHandler name="/blah"> to override
the path that each
+ // resource registers under?
+ Collection<Class<? extends JerseyResource>> jerseyApis =
+ apiSupport.getJerseyResources();
+ if (!CollectionUtils.isEmpty(jerseyApis)) {
+ for (Class<? extends JerseyResource> jerseyClazz : jerseyApis) {
+ if (log.isDebugEnabled()) {
+ log.debug("Registering jersey resource class: {}",
jerseyClazz.getName());
+ }
+ jerseyResources.register(jerseyClazz);
+ // See MetricsBeanFactory javadocs for a better understanding
of this resource->RH
+ // mapping
+ if (inst instanceof RequestHandlerBase) {
Review Comment:
I really appreciate your deeply thoughtful response!
Any way, this is just a smell; and there was some smell before to this
effect, albeit less.
FYI note that the package management system can dynamically add/change
handlers without even reloading the core. It remains to be seen if
JAX-RS/Jersey works with that system but I wouldn't invest time in that now.
##########
solr/core/src/java/org/apache/solr/jersey/MetricBeanFactory.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.jersey;
+
+import org.apache.solr.core.PluginBag;
+import org.glassfish.hk2.api.Factory;
+
+/**
+ * Factory to inject JerseyMetricsLookupRegistry instances into Jersey
resources and filters.
+ *
+ * <p>Currently, Jersey resources that have a corresponding v1 API produce the
same metrics as their
+ * v1 equivalent and rely on the v1 requestHandler instance to do so. Solr
facilitates this by
+ * building a map of the Jersey resource to requestHandler mapping (a {@link
+ * org.apache.solr.core.PluginBag.JerseyMetricsLookupRegistry}), and injecting
it into the pre- and
+ * post- Jersey filters that handle metrics.
+ *
+ * <p>This isn't ideal, as requestHandler's don't really "fit" conceptually
here. But it's
+ * unavoidable while we want our v2 APIs to exactly match the metrics produced
by v1 calls.
Review Comment:
> CollectionsHandler lumps together collection creation requests with
clusterstatus calls with shard-splits, etc. That needn't be the case if we
split from v1, which could be really cool.
Awesome!
Perhaps this is for Solr 10 then.
--
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]
