dsmiley commented on code in PR #3885:
URL: https://github.com/apache/solr/pull/3885#discussion_r2552287673
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -141,8 +144,14 @@ public void backdoorOffer() {
}
protected ConcurrentUpdateHttp2SolrClient(Builder builder) {
+ if (builder.baseSolrUrl == null) {
Review Comment:
moved from the build() method (the immediate caller)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJettySolrClient.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.client.solrj.impl;
Review Comment:
I want this in the right package (`jetty` not `impl) obviously. In this PR
I tried but then it couldn't access protected stuff in Http2SolrClient, which
has yet to be renamed and moved to that project. So this and that can move
together.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -177,7 +186,7 @@ protected ConcurrentUpdateHttp2SolrClient(Builder builder) {
}
/** Class representing an UpdateRequest and an optional collection. */
- private record Update(UpdateRequest request, String collection) {}
+ protected record Update(UpdateRequest request, String collection) {}
Review Comment:
precommit isn't happy because we have yet to solve a javadoc conundrum where
our javadoc config doesn't know what to do with a record! Not root caused yet;
I looked a month or two ago. When it was private, it wasn't published so was a
non-issue.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -47,29 +48,31 @@
import org.slf4j.LoggerFactory;
import org.slf4j.MDC;
-/** A Solr client using {@link Http2SolrClient} to send concurrent updates to
Solr. */
-public class ConcurrentUpdateHttp2SolrClient extends SolrClient {
+/** A ConcurrentUpdate {@link SolrClient} -- it sends updates concurrently and
asynchronously. */
+public abstract class ConcurrentUpdateHttp2SolrClient extends SolrClient {
+ // nocommit rename to ConcurrentUpdateBaseSolrClient or
ConcurrentUpdateSolrClientBase
Review Comment:
a nocommit for the rename. Lets pick a name.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -641,15 +633,16 @@ public void shutdownNow() {
}
/** Constructs {@link ConcurrentUpdateHttp2SolrClient} instances from
provided configuration. */
- public static class Builder {
- protected Http2SolrClient client;
+ public abstract static class Builder {
Review Comment:
not bothering with generics... but can change if asked
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java:
##########
@@ -340,6 +340,20 @@ protected void setParser(ResponseParser parser) {
protected abstract void updateDefaultMimeTypeForParser();
+ /**
+ * Executes a SolrRequest using the provided URL to temporarily override any
"base URL" currently
+ * used by this client
+ *
+ * @param baseUrl a URL to a root Solr path (i.e. "/solr") that should be
used for this request
+ * @param solrRequest the SolrRequest to send
+ * @param collection an optional collection or core name used to override
the client's "default
+ * collection". May be 'null' for any requests that don't require a
collection or wish to rely
+ * on the client's default
+ */
+ public abstract NamedList<Object> requestWithBaseUrl(
Review Comment:
from the other PR but it's very nice here. I'd like to see that one merged
ASAP.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateJettySolrClient.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.client.solrj.impl;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.util.Locale;
+import java.util.Objects;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.client.solrj.util.ClientUtils;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.params.UpdateParams;
+import org.eclipse.jetty.client.InputStreamResponseListener;
+import org.eclipse.jetty.client.OutputStreamRequestContent;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.http.HttpMethod;
+
+/** A ConcurrentUpdate SolrClient using {@link Http2SolrClient}. */
+public class ConcurrentUpdateJettySolrClient extends
ConcurrentUpdateHttp2SolrClient {
+ protected static final Charset FALLBACK_CHARSET = StandardCharsets.UTF_8;
+
+ private final Http2SolrClient client;
+
+ public static class Builder extends ConcurrentUpdateHttp2SolrClient.Builder {
+ /**
+ * @see ConcurrentUpdateHttp2SolrClient.Builder#Builder(String,
HttpSolrClientBase)
+ */
+ public Builder(String baseUrl, Http2SolrClient client) {
+ this(baseUrl, client, false);
+ }
+
+ /**
+ * @see ConcurrentUpdateHttp2SolrClient.Builder#Builder(String,
HttpSolrClientBase, boolean)
+ */
+ public Builder(String baseSolrUrl, Http2SolrClient client, boolean
closeHttpClient) {
+ super(baseSolrUrl, client, closeHttpClient);
+ this.idleTimeoutMillis = client.getIdleTimeoutMillis();
+ }
+
+ @Override
+ public ConcurrentUpdateJettySolrClient build() {
+ return new ConcurrentUpdateJettySolrClient(this);
+ }
+ }
+
+ protected ConcurrentUpdateJettySolrClient(Builder builder) {
+ super(builder);
+ this.client = (Http2SolrClient) builder.client;
+ }
+
+ @Override
+ protected InputStreamResponseListener doSendUpdateStream(
+ ConcurrentUpdateHttp2SolrClient.Update update) throws IOException,
InterruptedException {
+ InputStreamResponseListener responseListener;
+ try (OutStream out = initOutStream(basePath, update.request(),
update.collection())) {
+ ConcurrentUpdateHttp2SolrClient.Update upd = update;
+ while (upd != null) {
+ UpdateRequest req = upd.request();
+ if (!out.belongToThisStream(req, upd.collection())) {
+ // Request has different params or destination core/collection,
return to queue
+ queue.add(upd);
+ break;
+ }
+ send(out, upd.request(), upd.collection());
+ out.flush();
+
+ notifyQueueAndRunnersIfEmptyQueue();
+ upd = queue.poll(pollQueueTimeMillis, TimeUnit.MILLISECONDS);
+ }
+ responseListener = out.getResponseListener();
+ }
+ return responseListener;
+ }
+
+ private static class OutStream implements Closeable {
Review Comment:
From this point to the bottom, it's all moved from Http2SolrClient. Since
these methods are tightly related to concurrent updates, they seem more
appropriate here, and were distracting where they were IMO.
--
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]
