gerlowskija commented on code in PR #975:
URL: https://github.com/apache/solr/pull/975#discussion_r964814595
##########
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##########
@@ -362,14 +363,15 @@ private void invokeJerseyRequest(CoreContainer cores,
SolrCore core, Application
// Set properties that may be used by Jersey filters downstream
containerRequest.setProperty(SOLR_QUERY_REQUEST_KEY, solrReq);
-
containerRequest.setProperty(SolrRequestAuthorizer.CORE_CONTAINER_PROP_NAME,
cores);
-
containerRequest.setProperty(SolrRequestAuthorizer.HTTP_SERVLET_REQ_PROP_NAME,
req);
-
containerRequest.setProperty(SolrRequestAuthorizer.REQUEST_TYPE_PROP_NAME,
requestType);
-
containerRequest.setProperty(SolrRequestAuthorizer.SOLR_PARAMS_PROP_NAME,
queryParams);
-
containerRequest.setProperty(SolrRequestAuthorizer.COLLECTION_LIST_PROP_NAME,
collectionsList);
-
containerRequest.setProperty(SolrRequestAuthorizer.HTTP_SERVLET_RSP_PROP_NAME,
response);
+ containerRequest.setProperty(SOLR_QUERY_RESPONSE_KEY, rsp);
Review Comment:
Ah, looks like I missed the point 😬
I think the fully-qualified identifiers are from an IDE refactor and the
unqualified ones were added by hand. I'll add qualification across the board
here, to keep things consistent and as-per David's preference.
##########
solr/core/src/java/org/apache/solr/jersey/RequestEventLogger.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import org.glassfish.jersey.server.monitoring.RequestEvent;
+import org.glassfish.jersey.server.monitoring.RequestEventListener;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Logs out request-specific information useful for troubleshooting Jersey
development.
+ *
+ * @see ApplicationEventLogger
+ */
+public class RequestEventLogger implements RequestEventListener {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private volatile long requestCount;
Review Comment:
Happy to oblige if we do keep this class, though, see comment
[here](https://github.com/apache/solr/pull/975/files#r960826852)
##########
solr/core/src/java/org/apache/solr/jersey/ApplicationEventLogger.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import org.glassfish.jersey.server.monitoring.ApplicationEvent;
+import org.glassfish.jersey.server.monitoring.ApplicationEventListener;
+import org.glassfish.jersey.server.monitoring.RequestEvent;
+import org.glassfish.jersey.server.monitoring.RequestEventListener;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Logs out application-level information useful for troubleshooting Jersey
development.
+ *
+ * @see RequestEventLogger
+ */
+public class ApplicationEventLogger implements ApplicationEventListener {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private volatile long requestCount = 0;
+
+ @Override
+ public void onEvent(ApplicationEvent event) {
+ if (log.isInfoEnabled()) {
+ log.info("Received ApplicationEvent {}", event.getType());
+ }
+ }
+
+ @Override
+ public RequestEventListener onRequest(RequestEvent requestEvent) {
+ requestCount++;
+ log.info("Starting Jersey request {}", requestCount);
Review Comment:
ditto, re: my reply to Kevin
[here](https://github.com/apache/solr/pull/975/files#r960826852)
I'm not sure how useful these classes will be once the framework is in
place. I can keep them if you guys would like though
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -215,54 +216,58 @@ public void handleRequest(SolrQueryRequest req,
SolrQueryResponse rsp) {
}
}
} catch (Exception e) {
- if (req.getCore() != null) {
- boolean isTragic =
req.getCoreContainer().checkTragicException(req.getCore());
- if (isTragic) {
- if (e instanceof SolrException) {
- // Tragic exceptions should always throw a server error
- assert ((SolrException) e).code() == 500;
- } else {
- // wrap it in a solr exception
- e = new SolrException(SolrException.ErrorCode.SERVER_ERROR,
e.getMessage(), e);
- }
- }
- }
- boolean incrementErrors = true;
- boolean isServerError = true;
- if (e instanceof SolrException) {
- SolrException se = (SolrException) e;
- if (se.code() == SolrException.ErrorCode.CONFLICT.code) {
- incrementErrors = false;
- } else if (se.code() >= 400 && se.code() < 500) {
- isServerError = false;
- }
- } else {
- if (e instanceof SyntaxError) {
- isServerError = false;
- e = new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
- }
- }
-
+ e = normalizeReceivedException(req, e);
+ processErrorMetricsOnException(e, metrics);
rsp.setException(e);
+ } finally {
+ long elapsed = timer.stop();
+ metrics.totalTime.inc(elapsed);
+ }
+ }
- if (incrementErrors) {
- SolrException.log(log, e);
+ public static void processErrorMetricsOnException(Exception e,
HandlerMetrics metrics) {
+ boolean isClientError = false;
+ if (e instanceof SolrException) {
+ final SolrException se = (SolrException) e;
+ if (se.code() == SolrException.ErrorCode.CONFLICT.code) {
+ return;
+ } else if (se.code() >= 400 && se.code() < 500) {
+ isClientError = true;
+ }
+ }
- metrics.numErrors.mark();
- if (isServerError) {
- metrics.numServerErrors.mark();
+ SolrException.log(log, e);
+ metrics.numErrors.mark();
+ if (isClientError) {
+ metrics.numClientErrors.mark();
+ } else {
+ metrics.numServerErrors.mark();
+ }
+ }
+
+ public static Exception normalizeReceivedException(SolrQueryRequest req,
Exception e) {
+ if (req.getCore() != null) {
+ boolean isTragic =
req.getCoreContainer().checkTragicException(req.getCore());
Review Comment:
🙍 Bleh, really dislike all the 'lift' noise from code that was shifted
around but not really added in the PR. I get that even understanding what
lines are "new" is a hard problem, but bleh.
##########
solr/core/src/java/org/apache/solr/core/PluginBag.java:
##########
@@ -490,4 +532,8 @@ public Api v2lookup(String path, String method, Map<String,
String> parts) {
public ApiBag getApiBag() {
return apiBag;
}
+
+ public ResourceConfig getJerseyEndpoints() {
Review Comment:
I really dislike the name "ResourceConfig" that comes out of Jersey. So my
thought process here was to abstract away the bad name as much as we can by
giving this method a name that describes things more from a conceptual or
functional perspective. The main thing ResourceConfig does for us is wrap up
all of the APIs/endpoints we want available on a given core, etc, so
emphasizing that in the method name seemed like it'd be clearer than
"getResourceConfig" or even "getJerseyResourceConfig".
No rhyme or reason I didn't do the same thing to CoreContainer - I just
missed updating it when I renamed the method here. (In any case - I've deleted
the method off of CC in an upcoming commit, so consistency there is a bit moot.)
Anyway - I don't feel strongly about it. Happy to change or keep if you
have a preference.
##########
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##########
@@ -388,33 +335,65 @@ public CompositeApi add(Api api) {
}
}
+ // We don't rely on any of Jersey's authc/z features so we pass in this
empty context for
+ // all requests.
+ public static final SecurityContext DEFAULT_SECURITY_CONTEXT = new
SecurityContext() {
+ public boolean isUserInRole(String role) { return false; }
+ public boolean isSecure() { return false; }
+ public Principal getUserPrincipal() { return null; }
+ public String getAuthenticationScheme() { return null; }
+ };
+
+ private void invokeJerseyRequest(CoreContainer cores, SolrCore core,
ApplicationHandler jerseyHandler, SolrQueryResponse rsp) {
+ try {
+ final ContainerRequest containerRequest =
ContainerRequestUtils.createContainerRequest(req, response,
jerseyHandler.getConfiguration());
+
+ // Set properties that may be used by Jersey filters downstream
+ containerRequest.setProperty(SOLR_QUERY_REQUEST_KEY, solrReq);
+ containerRequest.setProperty(SOLR_QUERY_RESPONSE_KEY, rsp);
+ containerRequest.setProperty(RequestContextConstants.CORE_CONTAINER_KEY,
cores);
Review Comment:
It holds nothing but keys currently, but it's easy to imagine it holding
defaults or some other type of value down the road. I'd prefer to keep 'KEY'
in these names for now, as defense against that potential confusion.
I'll change if you feel strongly though.
##########
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:
It ended up turning into dead code as I went through the PR, so this PR
removes it. On `main` it handles some especially nested introspect APIs, but
some of the other changes to V2HttpCall made these invocations superfluous.
(As a larger question, I wonder whether we still see "introspect" APIs as
being valuable, or whether they could be replaced or reimplemented with some of
the tooling that an OpenAPI integration would offer us. But that's a
discussion for another day I think...)
##########
solr/core/src/java/org/apache/solr/jersey/ApplicationEventLogger.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 java.lang.invoke.MethodHandles;
+import org.glassfish.jersey.server.monitoring.ApplicationEvent;
+import org.glassfish.jersey.server.monitoring.ApplicationEventListener;
+import org.glassfish.jersey.server.monitoring.RequestEvent;
+import org.glassfish.jersey.server.monitoring.RequestEventListener;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Logs out application-level information useful for troubleshooting Jersey
development.
+ *
+ * @see RequestEventLogger
+ */
+public class ApplicationEventLogger implements ApplicationEventListener {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private volatile long requestCount = 0;
Review Comment:
Yeah, that'd be a good idea.
I actually only had this class (and its partner, RequestEventLogger) around
for my own personal debugging. I intended to delete them from the PR prior to
merging.
I'm happy to keep them around (and make the change you suggested above) if
you think they'd be a helpful aid to devs though?
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -184,7 +185,7 @@
* to make it work. When multi-core support was added to Solr way back in
version 1.3, this class
* was required so that the core functionality could be re-used multiple times.
*/
-public final class SolrCore implements SolrInfoBean, Closeable {
+public class SolrCore implements SolrInfoBean, Closeable {
Review Comment:
To be mock-able in tests.
See SchemaNameAPITest, as an example. The simplicity of that API maybe
doesn't show the value of being able to mock SolrCore's. (Maybe I should've
picked a more involved API to convert to Jersey as an example in this PR...)
Anyway, simplicity aside, you can probably imagine how being able to mock
SolrCores could be helpful for unit testing in a more involved API.
##########
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java:
##########
@@ -192,6 +200,28 @@ public Category getCategory() {
return Category.ADMIN;
}
+ public Boolean registerV2() {
+ return true;
+ }
+
+ @Override
+ public Collection<Api> getApis() {
Review Comment:
I'm going to skip `List.of` here - `AnnotatedApi.getApis` returns a
`List<Api>` itself, so there's a flattening problem with using `List.of` that's
more trouble than its worth.
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -215,54 +215,59 @@ public void handleRequest(SolrQueryRequest req,
SolrQueryResponse rsp) {
}
}
} catch (Exception e) {
- if (req.getCore() != null) {
- boolean isTragic =
req.getCoreContainer().checkTragicException(req.getCore());
- if (isTragic) {
- if (e instanceof SolrException) {
- // Tragic exceptions should always throw a server error
- assert ((SolrException) e).code() == 500;
- } else {
- // wrap it in a solr exception
- e = new SolrException(SolrException.ErrorCode.SERVER_ERROR,
e.getMessage(), e);
- }
- }
- }
- boolean incrementErrors = true;
- boolean isServerError = true;
- if (e instanceof SolrException) {
- SolrException se = (SolrException) e;
- if (se.code() == SolrException.ErrorCode.CONFLICT.code) {
- incrementErrors = false;
- } else if (se.code() >= 400 && se.code() < 500) {
- isServerError = false;
- }
- } else {
- if (e instanceof SyntaxError) {
- isServerError = false;
- e = new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
- }
- }
-
+ e = normalizeReceivedException(req, e);
+ processErrorMetricsOnException(e, metrics);
rsp.setException(e);
+ } finally {
+ long elapsed = timer.stop();
+ metrics.totalTime.inc(elapsed);
+ }
+ }
- if (incrementErrors) {
- SolrException.log(log, e);
+ public static void processErrorMetricsOnException(Exception e,
HandlerMetrics metrics) {
+ boolean isClientError = false;
+ if (e instanceof SolrException) {
+ final SolrException se = (SolrException) e;
+ if (se.code() == SolrException.ErrorCode.CONFLICT.code) {
+ return;
+ } else if (se.code() >= 400 && se.code() < 500) {
+ isClientError = true;
+ }
+ }
+
+ SolrException.log(log, e);
+ metrics.numErrors.mark();
+ if (isClientError) {
+ metrics.numClientErrors.mark();
+ } else {
+ metrics.numServerErrors.mark();
+ }
+ }
- metrics.numErrors.mark();
- if (isServerError) {
- metrics.numServerErrors.mark();
+ public static Exception normalizeReceivedException(SolrQueryRequest req,
Exception e) {
+ if (req.getCore() != null) {
+ assert req.getCoreContainer() != null;
+ boolean isTragic =
req.getCoreContainer().checkTragicException(req.getCore());
+ if (isTragic) {
+ if (e instanceof SolrException) {
+ // Tragic exceptions should always throw a server error
+ assert ((SolrException) e).code() == 500;
Review Comment:
I'm happy to add this, but this code pre-exists my PR and might change
pre-existing behavior. Figured it might not've been clear given the diff.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaNameResponse.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.handler.admin.api;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.solr.jersey.SolrJerseyResponse;
+
+public class GetSchemaNameResponse extends SolrJerseyResponse {
+ @JsonProperty("name")
+ public String name;
Review Comment:
We could go that route if you think there are advantages, but there's at
least one tradeoff. See the javadocs on
`JerseyResource.instantiateJerseyResponse`
[here](https://github.com/apache/solr/pull/975/files#diff-dc8c81d35faf30ab32e71cf916a001181724ea496ae28e5928890894e938d890R35)
for some context.
To attempt to summarize: an immutable response object would prevent our v2
APIs from returning the sort of "mixed" responses that Solr returns today on
some errors. I don't love this current behavior of Solr's - in fact I'd call
it a bug - but breaking compatibility with it would set us afoul of some tests
and bloat the diff here.
##########
solr/core/src/java/org/apache/solr/handler/configsets/ListConfigSetsAPI.java:
##########
@@ -16,34 +16,41 @@
*/
package org.apache.solr.handler.configsets;
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET;
import static
org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
-import java.util.List;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.client.solrj.SolrResponse;
-import org.apache.solr.cloud.OverseerSolrResponse;
-import org.apache.solr.common.util.NamedList;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.solr.api.JerseyResource;
import org.apache.solr.core.CoreContainer;
-import org.apache.solr.request.SolrQueryRequest;
-import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.jersey.PermissionName;
/**
* V2 API for adding or updating a single file within a configset.
*
* <p>This API (GET /v2/cluster/configs) is analogous to the v1
/admin/configs?action=LIST command.
*/
-public class ListConfigSetsAPI extends ConfigSetAPIBase {
+@Path("/cluster/configs")
+public class ListConfigSetsAPI extends JerseyResource {
+
+ @Context public HttpHeaders headers;
+
+ private final CoreContainer coreContainer;
+
+ @Inject
public ListConfigSetsAPI(CoreContainer coreContainer) {
- super(coreContainer);
+ this.coreContainer = coreContainer;
}
- @EndPoint(method = GET, path = "/cluster/configs", permission =
CONFIG_READ_PERM)
- public void listConfigSet(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
- final NamedList<Object> results = new NamedList<>();
- List<String> configSetsList = configSetService.listConfigs();
- results.add("configSets", configSetsList);
- SolrResponse response = new OverseerSolrResponse(results);
- rsp.getValues().addAll(response.getResponse());
+ @GET
+ @Produces({"application/json", "application/javabin"})
+ @PermissionName(CONFIG_READ_PERM)
+ public ListConfigsetsResponse listConfigSet() throws Exception {
+ final ListConfigsetsResponse response = new ListConfigsetsResponse();
+ response.configSets = coreContainer.getConfigSetService().listConfigs();
+ return response;
Review Comment:
See my reply to a similar comment
[here](https://github.com/apache/solr/pull/975/files#r963069506).
##########
solr/core/src/java/org/apache/solr/handler/configsets/ListConfigsetsResponse.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.handler.configsets;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.List;
+import org.apache.solr.jersey.SolrJerseyResponse;
+
+/** Response body POJO for the {@link ListConfigSetsAPI} resource. */
+public class ListConfigsetsResponse extends SolrJerseyResponse {
Review Comment:
Let's continue the discussion about immutability on other threads where you
mentioned it, but happy to move the class, sure.
##########
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:
Yes to both your points: (1) that this is sketchy code, and (2) that this is
related to the method-visibility change on RHB. I'm all for a better approach
here, but I struggled for some time to find one without any luck. Let me give
a little more context around what this code is trying to do, maybe you can see
an approach that I missed...
This code exists to serve our metrics integration.
Historically, v2 APIs got metrics "for free" because most v2 APIs were
"just" a translation layer around an existing requestHandler. Since the v2
APIs called `handler.handleRequest` at some point, the metrics code in
`RequestHandlerBase` would be invoked and metrics would "just work". Of
course, this only works for v2 APIs that have a v1 equivalent. Newer v2 APIs
(e.g. `ClusterAPI.roles`) never solved the metrics question afaict.
I don't like relying on RequestHandler for this functionality, but I thought
it _did_ made sense to have Jersey v2 APIs keep as close as possible to the
existing metrics setup. We'd be serving our users poorly if v1 and v2 requests
for the same functionality were tracked under different metrics. Or if the
metric names produced by v2 changed anytime we made an internal refactor to
move a v2 API over to Jersey. What a pain that'd be to make a dashboard for!
The way I thought to implement this for Jersey APIs was to give the Jersey
"resource" some pointer back to its relevant request handler, so it can
initialize its metrics. But this ended up being tricky, for two main reasons:
- In a sense, the `instanceof` check here is only necessary because we're
using the same container (`PluginBag`) for many different types of plugins. If
the type-param 'T' was always a RequestHandler, this code wouldn't have to do
nearly as much type-juggling. I thought about maybe creating a PluginBag
subclass specific to requestHandlers, but that seems to cut against the pretty
explicit intention here. It also steps on the toes of the `RequestHandlers`
class which I don't fully understand. IMO this is still the best option, but
it seems like a large refactor to drag into this (already large) PR.
- Other simpler solutions are off the table. For example, it'd be nice if
`ApiSupport.getJerseyResources` returned a collection of actual instances
instead of references to JerseyResource classes. That'd give each
RequestHandler a nice, strongly-typed place to give each resource instance the
metrics-context that they need. But this doesn't work for lifecycle reasons:
RequestHandlers generally live as long as the SolrCore or CoreContainer they
belong to, but Jersey generally instantiates a new resource instance for each
incoming request. (Jersey _does_ allow you to change this behavior, but doing
so would impose some restrictions that aren't worth the hassle IMO.) So
`getJerseyResources` has to return class references instead of nice
already-bootstrapped instances.
----
Anyway, hope this wasn't too long of a response. But I share your
irritation at this section, and I wanted to give you the context needed to
suggest an alternative if you can think of one. Really hoping your fresh eyes
will catch an approach I missed. I'm going to spend some time this afternoon
revisiting the issue as well.
##########
solr/core/src/java/org/apache/solr/handler/api/ApiRegistrar.java:
##########
@@ -79,12 +73,4 @@ public static void registerShardApis(ApiBag apiBag,
CollectionsHandler collectio
// here for simplicity.
apiBag.registerObject(new DeleteReplicaAPI(collectionsHandler));
}
-
- public static void registerConfigsetApis(ApiBag apiBag, CoreContainer
container) {
Review Comment:
It was merely moved, but it's tough to spot as it's in a totally different
file now: see `ConfigSetsHandler.getApis`.
In short: I created this method in ApiRegistrar during the apispec-framework
migration because at the time I thought container-level APIs couldn't be
registered via `SomeRequestHandler.getApis()` lookup, but I was wrong.
I plan to remove ApiRegistrar altogether in a subsequent commit, but since
this PR converts one of the configset APIs to JAX-RS, I decided to move those
APIs here-and-now.
##########
solr/core/src/java/org/apache/solr/api/JerseyResource.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.api;
+
+import static
org.apache.solr.jersey.RequestContextConstants.SOLR_JERSEY_RESPONSE_KEY;
+
+import java.util.function.Supplier;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.Context;
+import org.apache.solr.jersey.CatchAllExceptionMapper;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.servlet.HttpSolrCall;
+
+/** A marker parent type for all Jersey resource classes */
Review Comment:
Yes, it's a rough analog to v1's SolrRequestHandler (or v2's Api).
I've added these comparisons to the Javadocs here. Let me know if you think
it should get into more detail. It's definitely worth explaining, but at the
same time it's hard to give a just-thorough-enough Jersey/JAX-RS overview in
Javadocs. So if I didn't find the right balance, let me know.
##########
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:
I'd need to do a bit more research on the metric side to better understand
what'd make sense. I don't have a ton of experience working with Solr's
metrics, beyond setting up the prom exporter. That's another reason I was
reluctant to strike out and do something different with them in this PR.
The biggest change I can imagine from a user perspective would be that Solr
could provide metrics on a per-API granularity, instead of a per-RequestHandler
level.
e.g. Today, `CollectionsHandler` bundles together 15 or 20 different APIs,
some of which are only loosely related to one another. Since metrics are
RH-based, the request rate, total time, etc. metrics associated with
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.
In terms of the implementation, we'd still have the Pre- and Post- Jersey
filters that this PR introduces. But presumably the Post- filter would invoke
some sort of `recordMetrics(MetricsContext)` method defined by each
JerseyResource.
##########
solr/core/src/test/org/apache/solr/handler/configsets/ListConfigSetsAPITest.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * 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.handler.configsets;
+
+import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import javax.inject.Singleton;
+import javax.ws.rs.core.Application;
+import javax.ws.rs.core.Response;
+import org.apache.solr.client.solrj.response.ConfigSetAdminResponse;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.api.V2ApiUtils;
+import org.apache.solr.jersey.CoreContainerFactory;
+import org.apache.solr.jersey.SolrJacksonMapper;
+import org.glassfish.hk2.utilities.binding.AbstractBinder;
+import org.glassfish.jersey.server.ResourceConfig;
+import org.glassfish.jersey.test.JerseyTest;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ListConfigSetsAPITest extends JerseyTest {
+
+ private CoreContainer mockCoreContainer;
+
+ @BeforeClass
+ public static void ensureWorkingMockito() {
+ assumeWorkingMockito();
+ }
+
+ @Override
+ protected Application configure() {
+ resetMocks();
+ final ResourceConfig config = new ResourceConfig();
+ config.register(ListConfigSetsAPI.class);
+ config.register(SolrJacksonMapper.class);
+ config.register(
+ new AbstractBinder() {
+ @Override
+ protected void configure() {
+ bindFactory(new CoreContainerFactory(mockCoreContainer))
+ .to(CoreContainer.class)
+ .in(Singleton.class);
+ }
+ });
+
+ return config;
+ }
+
+ private void resetMocks() {
+ mockCoreContainer = mock(CoreContainer.class);
+ }
+
+ @Test
+ public void testSuccessfulListConfigsetsRaw() throws Exception {
+ final String expectedJson =
+
"{\"responseHeader\":{\"status\":0,\"QTime\":0},\"configSets\":[\"cs1\",\"cs2\"]}";
+ final ConfigSetService configSetService = mock(ConfigSetService.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ when(configSetService.listConfigs()).thenReturn(List.of("cs1", "cs2"));
+
+ final Response response = target("/cluster/configs").request().get();
+ final String jsonBody = response.readEntity(String.class);
+
+ assertEquals(200, response.getStatus());
+ assertEquals("application/json",
response.getHeaders().getFirst("Content-type"));
+ assertEquals(1, 1);
+ assertEquals(
+ expectedJson,
+
"{\"responseHeader\":{\"status\":0,\"QTime\":0},\"configSets\":[\"cs1\",\"cs2\"]}");
+ }
+
+ @Test
+ public void testSuccessfulListConfigsetsTyped() throws Exception {
+ final ConfigSetService configSetService = mock(ConfigSetService.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ when(configSetService.listConfigs()).thenReturn(List.of("cs1", "cs2"));
+
+ final ListConfigsetsResponse response =
+ target("/cluster/configs").request().get(ListConfigsetsResponse.class);
+
+ assertNotNull(response.configSets);
+ assertNull(response.error);
+ assertEquals(2, response.configSets.size());
+ assertTrue(response.configSets.contains("cs1"));
+ assertTrue(response.configSets.contains("cs2"));
+ }
+
+ /**
+ * Test the v2 to v1 response mapping for /cluster/configs
+ *
+ * <p>{@link org.apache.solr.handler.admin.ConfigSetsHandler} uses {@link
ListConfigSetsAPI} (and
+ * its response class {@link ListConfigsetsResponse}) internally to serve
the v1 version of this
+ * functionality. So it's important to make sure that the v2 response stays
compatible with SolrJ
+ * - both because that's important in its own right and because that ensures
we haven't
+ * accidentally changed the v1 response format.
+ */
+ @Test
+ public void testListConfigsetsV1Compatibility() throws Exception {
Review Comment:
Hah, the old integration vs unit test debate.
Each has a place at the table IMO. Integration tests do give you better
fidelity for high level functionality. I wouldn't rely on unit tests alone.
And unit tests do often need to change with the code, which is a pain.
But IMO unit tests have a lot of advantages: they're faster, more targeted,
run less risk of unrelated environmental issues (such as cause many of Solr's
flaky build issues), allow testing of otherwise hard-to-trigger codepaths, etc.
To answer your specific question about this test, I thought a unit test
would be a good addition for two reasons:
First, Solr has lots of integration tests around configset-listing, and I
trust those to flag issues with json/xml/javabin serialization if this were to
break. But none of them are very focused on serialization. They each leave a
pretty big gap between seeing the failure and understanding the root cause and
fixing it. This test addresses that by having something very narrowly focused
on just the response conversions involved, hopefully making a failure much more
actionable.
Second, I wanted to create an example test case based on a Jersey
test-application. The relative simplicity of the list-configset API doesn't
make the point very well, but more complex APIs will benefit a lot from
Jersey-framework-aided unit testing. And I wanted to include an example of
that in this PR.
##########
solr/core/src/test/org/apache/solr/handler/configsets/package-info.java:
##########
@@ -0,0 +1,19 @@
+/*
+ * 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.
+ */
+
+/** Testing for the org.apache.solr.handler.configsets package. */
+package org.apache.solr.handler.configsets;
Review Comment:
I only usually add these when `check` yells at me about it - so I fear it is
mandatory? I'll remove it and see though.
##########
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:
Neat, I'll try it out. Does anything stop folks from
extending/instantiating them the way the private-ctor does in a class?
##########
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:
Yep, there's definitely stuff in Solr responses that's not covered here,
yet. `echoParams` being a good example. But my thought was that those gaps
could be filled as we move forward, in our standard progress-not-perfection
fashion.
As we move v2 APIs to JAX-RS, we're going to hit tests that exercise e.g.
echoParams, and that'll serve as a forcing function to get coverage for those
additional fields here.
I can imagine some rigidity issues: like if we had two APIs that used the
same key within a responseHeader to store different types. But there are
tools/patterns for handling those as they come up, so IMO a strongly-typed
response was still worth trying.
But maybe I'm missing the specifics of your concern? Was it primarily
around incompleteness, or rigidity, or something else?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
