dsmiley commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r1931628768
##########
solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java:
##########
@@ -93,40 +114,66 @@ private void assertCoreChosen(int numCores, TestRequest
testRequest) throws Unav
assertEquals(numCores, coreNames.size());
}
- private static class TestResponse extends Response {
+ public static class TestRequest implements HttpServletRequest {
Review Comment:
I hate mocks but as I look at this, mocking seems perfect so you don't have
to implement everything. Or maybe there's a base class as there was before
that you can extend. (Same feedback for another source file you did this in)
##########
solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java:
##########
@@ -106,7 +106,7 @@ public static void setupCluster() throws Exception {
((Metered)
metricRegistry
.getMetrics()
-
.get("org.eclipse.jetty.servlet.ServletContextHandler.2xx-responses"))
+
.get("org.eclipse.jetty.server.Handler$Wrapper.2xx-responses"))
Review Comment:
This metrics name seems poor. I wonder if it's due to us/Solr (thus we have
easy control) or is this name chosen by Jetty / DropWizard.
##########
solr/modules/s3-repository/build.gradle:
##########
@@ -20,7 +20,9 @@ apply plugin: 'java-library'
description = 'S3 Repository'
dependencies {
- api project(':solr:core')
+ api(project(':solr:core')) {
+ exclude group: 'org.ow2.asm', module: '*'
Review Comment:
them solr:core shouldn't be publishing that dependency in the first place.
At least see if that can be done.
##########
solr/cross-dc-manager/src/java/org/apache/solr/crossdc/manager/consumer/Consumer.java:
##########
@@ -21,8 +21,8 @@
import static
org.apache.solr.crossdc.common.KafkaCrossDcConf.ZK_CONNECT_STRING;
import com.codahale.metrics.SharedMetricRegistries;
-import com.codahale.metrics.servlets.MetricsServlet;
-import com.codahale.metrics.servlets.ThreadDumpServlet;
+import io.dropwizard.metrics.servlets.MetricsServlet;
Review Comment:
it's suspicious to see we import com.codahale & io.dropwizard. Maybe okay
but I wasn't expecting.
##########
solr/server/etc/jetty.xml:
##########
@@ -214,14 +212,16 @@
<Set name="contexts">
<Ref refid="Contexts" />
</Set>
+ <!--
Review Comment:
appears to simply be some useless metadata we can remove
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CoreAdminAPIBase.java:
##########
@@ -90,7 +90,11 @@ public <T extends SolrJerseyResponse> T
handlePotentiallyAsynchronousTask(
}
MDCLoggingContext.setCoreName(coreName);
- TraceUtils.setDbInstance(req, coreName);
+ try {
+ TraceUtils.setDbInstance(req, coreName);
+ } catch (Exception e) {
+ // TODO: Find better way to fix NPE
Review Comment:
we use "nocommit" which will also fail precommit until we get it fixed,
serving as a reminder. Any way, this try-catch definitely shouldn't be here if
it should be anywhere. We should fix what's null instead.
##########
solr/core/src/test/org/apache/solr/util/tracing/TestHttpServletRequestGetter.java:
##########
@@ -40,8 +49,9 @@ public void test() {
"b", Set.of("1"),
"c", Set.of("2"));
+ HttpServletRequest httpServletRequestMock = mock(HttpServletRequest.class);
Review Comment:
aha; you figured out mocking :-)
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/JettyWebappTest.java:
##########
@@ -60,9 +60,9 @@ public void setUp() throws Exception {
server = new Server(port);
// insecure: only use for tests!!!!
- server.setSessionIdManager(
- new DefaultSessionIdManager(server, new Random(random().nextLong())));
- new WebAppContext(server, path, "/solr");
+ server.addBean(new DefaultSessionIdManager(server, new
Random(random().nextLong())));
+ var webAppContext = new WebAppContext(path, "/solr");
Review Comment:
minor: inline
##########
solr/test-framework/build.gradle:
##########
@@ -65,16 +65,19 @@ dependencies {
implementation libs.apache.log4j.api
implementation libs.apache.log4j.core
implementation libs.dropwizard.metrics.core
- implementation libs.dropwizard.metrics.jetty10
+ implementation (libs.dropwizard.metrics.jetty12.ee10){
+ exclude group: 'jakarta.servlet', module: 'jakarta.servlet-api'
Review Comment:
as long as we explicitly depend on `libs.jakarta.servlet.api` below (we do),
I wonder do we really need to exclude it here?
--
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
