dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1929924679
##########
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##########
@@ -282,7 +282,6 @@ private void commitOnLeader(String leaderBaseUrl, String
coreName)
throws SolrServerException, IOException {
try (SolrClient client = recoverySolrClientBuilder(leaderBaseUrl,
coreName).build()) {
UpdateRequest ureq = new UpdateRequest();
- ureq.setParams(new ModifiableSolrParams());
Review Comment:
This was clearly a hack to work around it's weird mutability situation. Now
not needed here and elsewhere.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java:
##########
@@ -20,13 +20,15 @@
import
org.apache.solr.client.solrj.request.RequestWriter.StringPayloadContentWriter;
import org.apache.solr.client.solrj.response.UpdateResponse;
import org.apache.solr.client.solrj.util.ClientUtils;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
/**
* Send arbitrary XML to a request handler
*
* @since solr 1.3
*/
+@Deprecated
Review Comment:
There's a lot I want to deprecate and remove in Solr 10; this is one. The
annotation snuck in; arguably for another PR. On the other hand, Solr isn't
using or testing this class, so let's get on with removing!
##########
solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java:
##########
@@ -625,11 +625,8 @@ protected long addDocAndGetVersion(Object... fields)
throws Exception {
SolrInputDocument doc = new SolrInputDocument();
addFields(doc, fields);
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.add("versions", "true");
-
UpdateRequest ureq = new UpdateRequest();
- ureq.setParams(params);
+ ureq.getParams().set("versions", true);
Review Comment:
With a clear mutability story, the requirement here is now easy with this
one-liner. (same elsewhere)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -380,7 +380,7 @@ public InputStreamResponseListener getResponseListener() {
public OutStream initOutStream(String baseUrl, UpdateRequest updateRequest,
String collection)
throws IOException {
String contentType = requestWriter.getUpdateContentType();
- final ModifiableSolrParams origParams = new
ModifiableSolrParams(updateRequest.getParams());
+ final SolrParams origParams = updateRequest.getParams();
Review Comment:
My suspicion is that `new ModifiableSolrParams` is here to implicilty handle
null (as it handles that in its constructor). We have no need to modify the
result, I confirm.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/AbstractUpdateRequest.java:
##########
@@ -21,10 +21,9 @@
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.UpdateParams;
-/** */
public abstract class AbstractUpdateRequest extends
CollectionRequiringSolrRequest<UpdateResponse>
implements IsUpdateRequest {
- protected ModifiableSolrParams params;
+ protected ModifiableSolrParams params = new ModifiableSolrParams(); // TODO
make final
Review Comment:
Up for debate on eventually final. That means we lose the setter, which is
called in a ton of places, in lieu of calling `getParams().add(params)`. I
actually went down that path and shelved the work. I was motivated for clarity
on the getParams value never being potentially shared with something else.
##########
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroredSolrRequest.java:
##########
@@ -72,7 +71,7 @@ public SolrParams getParams() {
return params;
}
- public void setParams(ModifiableSolrParams params) {
+ public void setParams(SolrParams params) {
Review Comment:
getter & setter should have same type
##########
solr/core/src/test/org/apache/solr/update/processor/TemplateUpdateProcessorTest.java:
##########
@@ -69,14 +71,11 @@ public void testSimple() throws Exception {
SolrInputDocument solrDoc = new SolrInputDocument();
solrDoc.addField("id", "1");
- params =
- new ModifiableSolrParams()
- .add("processor", "template")
- .add("commit", "true")
- .add("template.field", "x_s:key_{id}");
- params.add("commit", "true");
UpdateRequest add = new UpdateRequest().add(solrDoc);
- add.setParams(params);
+ add.getParams()
+ .add("processor", "template")
+ .add("template.field", "x_s:key_{id}")
+ .add("commit", "true");
Review Comment:
so nice IMO
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java:
##########
@@ -16,36 +16,39 @@
*/
package org.apache.solr.client.solrj.request;
+import java.util.Objects;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
/**
* @since solr 1.3
*/
public class QueryRequest extends
CollectionRequiringSolrRequest<QueryResponse> {
- private SolrParams query;
+ private final SolrParams query;
public QueryRequest() {
super(METHOD.GET, null);
+ query = new ModifiableSolrParams(); // TODO SolrParams.of()
Review Comment:
at some point I eventually added SolrParams.of; TODO to use it in more places
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/DocumentAnalysisRequest.java:
##########
@@ -85,7 +86,7 @@ protected DocumentAnalysisResponse createResponse(SolrClient
client) {
}
@Override
- public ModifiableSolrParams getParams() {
+ public SolrParams getParams() {
Review Comment:
With new clarity from the Javadocs, we should never declare the return type
as ModifiableSolrParams unless we have a consistent instance to return.
##########
solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java:
##########
@@ -647,10 +644,10 @@ protected long addDocAndGetVersion(Object... fields)
throws Exception {
protected long deleteDocAndGetVersion(
String id, ModifiableSolrParams params, boolean deleteByQuery) throws
Exception {
- params.add("versions", "true");
-
UpdateRequest ureq = new UpdateRequest();
- ureq.setParams(params);
+ ureq.getParams().add(params);
Review Comment:
I chose to call `SolrParams.add(SolrParams)` here but could have called
setParams. A nice effect of this is that we copy the params instead of use
in-place. Previously this deleteDocAndGetVersion method actually added params
to its argument, which is very bad taste.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -188,6 +188,13 @@ public void setQueryParams(Set<String> queryParams) {
/** This method defines the type of this Solr request. */
public abstract String getRequestType();
+ /**
+ * The parameters for this request; never null. The runtime type may be
mutable but modifications
+ * <b>may</b> not affect this {@link SolrRequest} instance, as it may return
a new instance here
+ * every time. If the subclass specifies the response type as {@link
+ * org.apache.solr.common.params.ModifiableSolrParams}, then one can expect
it to change this
+ * request. If the subclass has a setter then one can expect this method to
return the value set.
+ */
Review Comment:
This is where this whole PR started.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -128,7 +127,7 @@ public UpdateRequest unmarshal(InputStream is, final
StreamingUpdateHandler hand
// NOTE: if the update request contains only delete commands the params
// must be loaded now
- if (updateRequest.getParams() == null) {
+ if (updateRequest.getParams().iterator().hasNext() == false) { // no params
Review Comment:
This check is evil; took a while to find it as I debugged a test for hours.
Behavior is changing based on the presence of params or not. Like nowhere is
there a similar check (at least based on my adventure with this PR). The
history behind these lines of code is what I consider to be an ugly/messy
solution to propagating a commitWithin param. Tempting to add that on my long
TODO list of messes to fix but our longer future is not "javabin", and the mess
thankfully seems localized here.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericV2SolrRequest.java:
##########
@@ -40,8 +40,7 @@ public GenericV2SolrRequest(METHOD m, String path) {
* @param params query parameter names and values for making this request.
*/
public GenericV2SolrRequest(METHOD m, String path, SolrParams params) {
- super(m, path);
- this.params = params;
+ super(m, removeLeadingApiRoot(path), params);
Review Comment:
@gerlowskija this includes a minor bug fix (to call removeLeadingApiRoot as
the other overloaded constructor does) but maybe this code wasn't used
--
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]
