dsmiley commented on code in PR #3708:
URL: https://github.com/apache/solr/pull/3708#discussion_r2553404941
##########
gradle/java/javac.gradle:
##########
Review Comment:
To help prevent you committing stuff like this accidentally, I recommend the
use of IntelliJ changelists, and naming one "ignored". I routinely have stuff
in my ignored.
##########
solr/core/src/test/org/apache/solr/TestCrossCoreJoin.java:
##########
@@ -98,63 +99,34 @@ public static void beforeTests() throws Exception {
"Engineering")));
assertU(commit());
- update(
- fromCore,
- add(
- doc(
- "id",
- "10",
- "id_s_dv",
- "10",
- "dept_id_s",
- "Engineering",
- "text",
- "These guys develop stuff",
- "cat",
- "dev")));
- update(
- fromCore,
- add(
- doc(
- "id",
- "11",
- "id_s_dv",
- "11",
- "dept_id_s",
- "Marketing",
- "text",
- "These guys make you look good")));
- update(
- fromCore,
- add(
- doc(
- "id",
- "12",
- "id_s_dv",
- "12",
- "dept_id_s",
- "Sales",
- "text",
- "These guys sell stuff")));
- update(
- fromCore,
- add(
- doc(
- "id",
- "13",
- "id_s_dv",
- "13",
- "dept_id_s",
- "Support",
- "text",
- "These guys help customers")));
- update(fromCore, commit());
- }
-
- public static String update(SolrCore core, String xml) throws Exception {
- DirectSolrConnection connection = new DirectSolrConnection(core);
- SolrRequestHandler handler = core.getRequestHandler("/update");
- return connection.request(handler, null, xml);
+ // Add documents to the fromCore
+ List<SolrInputDocument> docs =
+ sdocs(
Review Comment:
that method is so pointless but whatever
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -379,12 +391,36 @@ public void testResponse() throws SAXException,
XPathExpressionException, IOExce
}
public String update(String chain, String xml) {
- DirectSolrConnection connection = new DirectSolrConnection(h.getCore());
- SolrRequestHandler handler = h.getCore().getRequestHandler("/update");
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.add("update.chain", chain);
try {
- return connection.request(handler, params, xml);
+ // Use DirectXmlRequest to send raw XML through EmbeddedSolrServer
+ DirectXmlRequest xmlRequest = new DirectXmlRequest("/update", xml);
Review Comment:
this is deprecated. It's the kind of thing that would only really be used
in tests if anywhere.
##########
solr/test-framework/src/java/org/apache/solr/util/TestHarness.java:
##########
@@ -284,14 +283,28 @@ public String update(String xml) {
try (var mdcSnap = MDCSnapshot.create();
SolrCore core = getCoreInc()) {
assert null != mdcSnap; // prevent compiler warning of unused var
- DirectSolrConnection connection = new DirectSolrConnection(core);
- SolrRequestHandler handler = core.getRequestHandler("/update");
- // prefer the handler mapped to /update, but use our generic backup
handler
- // if that lookup fails
- if (handler == null) {
- handler = updater;
+
Review Comment:
deja-vu; I already commented on this code, repeated
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -57,12 +61,20 @@ public class TolerantUpdateProcessorTest extends
UpdateProcessorTestBase {
@BeforeClass
public static void beforeClass() throws Exception {
initCore("solrconfig-update-processor-chains.xml", "schema12.xml");
+ server = new EmbeddedSolrServer(h.getCoreContainer(),
h.getCore().getName());
}
@AfterClass
- public static void tearDownClass() {
+ public static void afterClass() {
docs = null;
- badIds = null;
+ if (server != null) {
Review Comment:
no need. `server` is an `EmbeddedSolrServer` constructed with a
`CoreContainer` originating from `TestHarness`. It's a thin veneer on
something whose lifecycle is already managed (i.e. meaning, it's closed).
It'd be nice if TestHarness had a `client` field (I'd call it that, not
server) so that tests cold use the familiar SolrClient abstraction. Granted I
want TestHarness to go away entirely, so... :shrug:
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -379,12 +391,36 @@ public void testResponse() throws SAXException,
XPathExpressionException, IOExce
}
public String update(String chain, String xml) {
Review Comment:
sad to see a method like this with a completely unclear response
##########
solr/core/src/test/org/apache/solr/TestCrossCoreJoin.java:
##########
@@ -17,40 +17,41 @@
package org.apache.solr;
import java.util.Collections;
+import java.util.List;
import java.util.Map;
+import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer;
import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.core.SolrCore;
import org.apache.solr.request.LocalSolrQueryRequest;
import org.apache.solr.request.SolrQueryRequest;
-import org.apache.solr.request.SolrRequestHandler;
import org.apache.solr.request.SolrRequestInfo;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.search.join.TestScoreJoinQPNoScore;
-import org.apache.solr.servlet.DirectSolrConnection;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
public class TestCrossCoreJoin extends SolrTestCaseJ4 {
private static SolrCore fromCore;
+ private static EmbeddedSolrServer fromServer;
@BeforeClass
public static void beforeTests() throws Exception {
System.setProperty(
"solr.index.updatelog.enabled", "false"); // schema12 doesn't support
_version_
System.setProperty("solr.filterCache.async", "true");
- // initCore("solrconfig.xml","schema12.xml");
- // File testHome = createTempDir().toFile();
- // FileUtils.copyDirectory(getFile("solrj/solr"), testHome);
initCore("solrconfig.xml", "schema12.xml", TEST_HOME(), "collection1");
final CoreContainer coreContainer = h.getCoreContainer();
fromCore = coreContainer.create("fromCore", Map.of("configSet",
"minimal"));
+ fromServer = new EmbeddedSolrServer(fromCore.getCoreContainer(),
fromCore.getName());
Review Comment:
it's more idiomatic to get it from TestHarness `h`
##########
solr/core/src/test/org/apache/solr/update/processor/TolerantUpdateProcessorTest.java:
##########
@@ -379,12 +391,36 @@ public void testResponse() throws SAXException,
XPathExpressionException, IOExce
}
public String update(String chain, String xml) {
- DirectSolrConnection connection = new DirectSolrConnection(h.getCore());
- SolrRequestHandler handler = h.getCore().getRequestHandler("/update");
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.add("update.chain", chain);
try {
- return connection.request(handler, params, xml);
+ // Use DirectXmlRequest to send raw XML through EmbeddedSolrServer
+ DirectXmlRequest xmlRequest = new DirectXmlRequest("/update", xml);
+
+ // Set the update chain parameter
+ ModifiableSolrParams params = new ModifiableSolrParams();
+ params.add("update.chain", chain);
+ params.add("wt", "xml");
+ xmlRequest.setParams(params);
+
+ // Process the request and get the response
+ NamedList<Object> response = server.request(xmlRequest);
Review Comment:
if the goal is to get a Solr XML response, then just use
InputStreamResponseParser. Do away with everything that follows, which is a
headache
##########
solr/core/src/test/org/apache/solr/TestCrossCoreJoin.java:
##########
@@ -245,6 +217,14 @@ public String query(SolrCore core, SolrQueryRequest req)
throws Exception {
@AfterClass
public static void nukeAll() {
+ if (fromServer != null) {
Review Comment:
again; ditch this -- needless
##########
solr/core/src/test/org/apache/solr/TestCrossCoreJoin.java:
##########
@@ -245,6 +218,14 @@ public String query(SolrCore core, SolrQueryRequest req)
throws Exception {
@AfterClass
public static void nukeAll() {
+ if (fromServer != null) {
Review Comment:
albeit we don't need to close EmbeddedSolrServer when constructed from an
_existing_ CoreContainer
--
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]
