Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SolrRequest.getParams never null; and clarify mutability #3140

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was clearly a hack to work around it's weird mutability situation. Now not needed here and elsewhere.

// ureq.getParams().set(DistributedUpdateProcessor.COMMIT_END_POINT, true);
// ureq.getParams().set(UpdateParams.OPEN_SEARCHER, onlyLeaderIndexes);
// Why do we need to open searcher if "onlyLeaderIndexes"?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@ static UpdateResponse softCommit(String baseUrl, String coreName)
.withSocketTimeout(120000, TimeUnit.MILLISECONDS)
.build()) {
UpdateRequest ureq = new UpdateRequest();
ureq.setParams(new ModifiableSolrParams());
ureq.setAction(AbstractUpdateRequest.ACTION.COMMIT, false, true, true);
return ureq.process(client);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
}
try {
SolrParams params = req.getParams();
assert params != null;
UpdateRequestProcessorChain processorChain = req.getCore().getUpdateProcessorChain(params);

UpdateRequestProcessor processor = processorChain.createProcessor(req, rsp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ static Long getCheckSum(Checksum checksum, Path f) {
private volatile IndexFetcher currentIndexFetcher;

public IndexFetchResult doFetch(SolrParams solrParams, boolean forceReplication) {
String leaderUrl = solrParams == null ? null : solrParams.get(LEADER_URL, null);
String leaderUrl = solrParams.get(LEADER_URL, null);
if (!indexFetchLock.tryLock()) return IndexFetchResult.LOCK_OBTAIN_FAILED;
if (core.getCoreContainer().isShutDown()) {
log.warn("I was asked to replicate but CoreContainer is shutting down");
Expand Down Expand Up @@ -1196,7 +1196,7 @@ private void setupPolling(String intervalStr) {
try {
log.debug("Polling for index modifications");
markScheduledExecutionStart();
IndexFetchResult fetchResult = doFetch(null, false);
IndexFetchResult fetchResult = doFetch(SolrParams.of(), false);
if (pollListener != null) pollListener.onComplete(core, fetchResult);
} catch (Exception e) {
log.error("Exception in fetching index", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
Expand Down Expand Up @@ -53,10 +52,6 @@ public static void addExperimentalFormatWarning(SolrQueryResponse rsp) {
public static boolean handleCommit(
SolrQueryRequest req, UpdateRequestProcessor processor, SolrParams params, boolean force)
throws IOException {
if (params == null) {
params = new MapSolrParams(new HashMap<>());
}

boolean optimize = params.getBool(UpdateParams.OPTIMIZE, false);
boolean commit = params.getBool(UpdateParams.COMMIT, false);
boolean softCommit = params.getBool(UpdateParams.SOFT_COMMIT, false);
Expand Down Expand Up @@ -97,8 +92,6 @@ public static void validateCommitParams(SolrParams params) {

/** Modify UpdateCommand based on request parameters */
public static void updateCommit(CommitUpdateCommand cmd, SolrParams params) {
if (params == null) return;

cmd.openSearcher = params.getBool(UpdateParams.OPEN_SEARCHER, cmd.openSearcher);
cmd.waitSearcher = params.getBool(UpdateParams.WAIT_SEARCHER, cmd.waitSearcher);
cmd.softCommit = params.getBool(UpdateParams.SOFT_COMMIT, cmd.softCommit);
Expand All @@ -114,13 +107,7 @@ public static void updateCommit(CommitUpdateCommand cmd, SolrParams params) {
public static boolean handleRollback(
SolrQueryRequest req, UpdateRequestProcessor processor, SolrParams params, boolean force)
throws IOException {
if (params == null) {
params = new MapSolrParams(new HashMap<>());
}

boolean rollback = params.getBool(UpdateParams.ROLLBACK, false);

if (rollback || force) {
if (force || params.getBool(UpdateParams.ROLLBACK, false)) {
RollbackUpdateCommand cmd = new RollbackUpdateCommand(req);
processor.processRollback(cmd);
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ public static void squashIntoNamedListWithoutHeader(
}

public static String getMediaTypeFromWtParam(SolrParams params, String defaultMediaType) {
if (params == null) {
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
return defaultMediaType;
}
final String wtParam = params.get(WT);
if (StrUtils.isBlank(wtParam)) return defaultMediaType;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,6 @@ public SampleDocuments parseDocsFromStream(
return SampleDocuments.NONE;
}

if (params == null) {
params = new ModifiableSolrParams();
}

Long streamSize = stream.getSize();
if (streamSize != null && streamSize > MAX_STREAM_SIZE) {
throw new SolrException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ public class IgnoreCommitOptimizeUpdateProcessorFactory extends UpdateRequestPro

@Override
public void init(final NamedList<?> args) {
SolrParams params = (args != null) ? args.toSolrParams() : null;
if (params == null) {
if (args == null) {
errorCode = ErrorCode.FORBIDDEN; // default is 403 error
responseMsg = DEFAULT_RESPONSE_MSG;
ignoreOptimizeOnly = false;
return;
}
SolrParams params = args.toSolrParams();

ignoreOptimizeOnly = params.getBool("ignoreOptimizeOnly", false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.cloud.Replica;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -241,8 +240,6 @@ protected void indexDoc(Object... fields) throws IOException, SolrServerExceptio

UpdateRequest ureq = new UpdateRequest();
ureq.add(doc);
ModifiableSolrParams params = new ModifiableSolrParams();
ureq.setParams(params);
ureq.process(cloudClient);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.cloud.Replica;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.metrics.SolrMetricManager;
import org.junit.Test;
Expand Down Expand Up @@ -369,8 +368,6 @@ protected void indexDoc(Object... fields) throws IOException, SolrServerExceptio

UpdateRequest ureq = new UpdateRequest();
ureq.add(doc);
ModifiableSolrParams params = new ModifiableSolrParams();
ureq.setParams(params);
ureq.process(cloudClient);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a clear mutability story, the requirement here is now easy with this one-liner. (same elsewhere)

ureq.add(doc);
UpdateResponse resp;

Expand All @@ -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);
Comment on lines -653 to +648
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ureq.getParams().set("versions", true);

if (deleteByQuery) {
ureq.deleteByQuery("id:" + id);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void setup() throws IOException {

@Test
public void testJson() throws Exception {
loadTestDocs(null, new File(exampleDir, "films/films.json"), 500, 500);
loadTestDocs(SolrParams.of(), new File(exampleDir, "films/films.json"), 500, 500);
}

@Test
Expand All @@ -78,7 +78,7 @@ public void testCsv() throws Exception {

@Test
public void testSolrXml() throws Exception {
loadTestDocs(null, new File(exampleDir, "films/films.xml"), 1000, 1000);
loadTestDocs(SolrParams.of(), new File(exampleDir, "films/films.xml"), 1000, 1000);
}

protected List<SolrInputDocument> loadTestDocs(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@ public void read(SolrQueryRequest req, SolrQueryResponse rsp) {

private static SolrQueryResponse v2ApiInvoke(
ApiBag bag, String uri, String method, SolrParams params, InputStream payload) {
if (params == null) params = new ModifiableSolrParams();
SolrQueryResponse rsp = new SolrQueryResponse();
HashMap<String, String> templateVals = new HashMap<>();
Api[] currentApi = new Api[1];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,15 @@ public void after() throws Exception {

public void testSimple() throws Exception {

ModifiableSolrParams params =
new ModifiableSolrParams()
.add("processor", "template")
.add("template.field", "id:{firstName}_{lastName}")
.add("template.field", "another:{lastName}_{firstName}")
.add("template.field", "missing:{lastName}_{unKnown}");
AddUpdateCommand cmd = new AddUpdateCommand(new LocalSolrQueryRequest(null, params));
var cmd =
new AddUpdateCommand(
new LocalSolrQueryRequest(
null,
new ModifiableSolrParams()
.add("processor", "template")
.add("template.field", "id:{firstName}_{lastName}")
.add("template.field", "another:{lastName}_{firstName}")
.add("template.field", "missing:{lastName}_{unKnown}")));

cmd.solrDoc = new SolrInputDocument();
cmd.solrDoc.addField("firstName", "Tom");
Expand All @@ -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");
Comment on lines +75 to +78
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so nice IMO

NamedList<Object> result =
cluster
.getSolrClient()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public void testInvalidAdds() throws IOException {
assertAddsSucceedWithErrors(
"tolerant-chain-max-errors-10",
Arrays.asList(new SolrInputDocument[] {invalidDoc1}),
null,
SolrParams.of(),
"(unknown)");

// a valid doc
Expand All @@ -149,7 +149,7 @@ public void testInvalidAdds() throws IOException {
() ->
add(
"not-tolerant",
null,
SolrParams.of(),
Arrays.asList(new SolrInputDocument[] {invalidDoc1, validDoc1})));
assertTrue(e.getMessage().contains("Document is missing mandatory uniqueKey field"));

Expand All @@ -159,7 +159,7 @@ public void testInvalidAdds() throws IOException {
assertAddsSucceedWithErrors(
"tolerant-chain-max-errors-10",
Arrays.asList(new SolrInputDocument[] {invalidDoc1, validDoc1}),
null,
SolrParams.of(),
"(unknown)");
assertU(commit());

Expand All @@ -176,7 +176,7 @@ public void testInvalidAdds() throws IOException {
() ->
add(
"not-tolerant",
null,
SolrParams.of(),
Arrays.asList(new SolrInputDocument[] {invalidDoc2, validDoc2})));
assertTrue(e.getMessage().contains("Error adding field"));

Expand All @@ -186,7 +186,7 @@ public void testInvalidAdds() throws IOException {
assertAddsSucceedWithErrors(
"tolerant-chain-max-errors-10",
Arrays.asList(new SolrInputDocument[] {invalidDoc2, validDoc2}),
null,
SolrParams.of(),
"2");
assertU(commit());

Expand All @@ -201,7 +201,7 @@ public void testInvalidAdds() throws IOException {
public void testMaxErrorsDefault() throws IOException {
// by default the TolerantUpdateProcessor accepts all errors, so this batch should succeed with
// 10 errors.
assertAddsSucceedWithErrors("tolerant-chain-max-errors-not-set", docs, null, badIds);
assertAddsSucceedWithErrors("tolerant-chain-max-errors-not-set", docs, SolrParams.of(), badIds);
assertU(commit());
assertQ(req("q", "*:*"), "//result[@numFound='10']");
}
Expand Down Expand Up @@ -240,7 +240,7 @@ public void testMaxErrorsThrowsException() {
public void testMaxErrorsInfinite() throws IOException {
ModifiableSolrParams requestParams = new ModifiableSolrParams();
requestParams.add("maxErrors", "-1");
assertAddsSucceedWithErrors("tolerant-chain-max-errors-not-set", docs, null, badIds);
assertAddsSucceedWithErrors("tolerant-chain-max-errors-not-set", docs, SolrParams.of(), badIds);

assertU(commit());
assertQ(req("q", "*:*"), "//result[@numFound='10']");
Expand Down Expand Up @@ -421,7 +421,7 @@ private void assertAddsSucceedWithErrors(

protected SolrQueryResponse add(final String chain, final SolrInputDocument doc)
throws IOException {
return add(chain, null, Arrays.asList(new SolrInputDocument[] {doc}));
return add(chain, SolrParams.of(), Arrays.asList(new SolrInputDocument[] {doc}));
}

protected SolrQueryResponse add(
Expand All @@ -435,10 +435,6 @@ protected SolrQueryResponse add(
SolrQueryResponse rsp = new SolrQueryResponse();
rsp.add("responseHeader", new SimpleOrderedMap<>());

if (requestParams == null) {
requestParams = new ModifiableSolrParams();
}

SolrQueryRequest req = new LocalSolrQueryRequest(core, requestParams);
UpdateRequestProcessor processor = null;
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,8 @@ void preventCircularMirroring(MirroredSolrRequest<?> mirroredSolrRequest) {
}
} else {
SolrParams params = mirroredSolrRequest.getSolrRequest().getParams();
String shouldMirror = (params == null ? null : params.get(CrossDcConstants.SHOULD_MIRROR));
assert params != null;
String shouldMirror = params.get(CrossDcConstants.SHOULD_MIRROR);
if (shouldMirror == null) {
if (params instanceof ModifiableSolrParams) {
log.warn("{} param is missing - setting to false", CrossDcConstants.SHOULD_MIRROR);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.ExecutorUtil;
import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.common.util.Utils;
Expand Down Expand Up @@ -279,7 +278,7 @@ public void testHandleValidMirroredSolrRequest() {
doc.addField("id", "1");
UpdateRequest validRequest = new UpdateRequest();
validRequest.add(doc);
validRequest.setParams(new ModifiableSolrParams().add("commit", "true"));
validRequest.getParams().set("commit", true);
// Create a valid MirroredSolrRequest
ConsumerRecord<String, MirroredSolrRequest<?>> record =
new ConsumerRecord<>("test-topic", 0, 0, "key", new MirroredSolrRequest<>(validRequest));
Expand Down Expand Up @@ -459,7 +458,7 @@ protected KafkaMirroringSink createKafkaMirroringSink(KafkaCrossDcConf conf) {

UpdateRequest invalidRequest = new UpdateRequest();
// no updates on request
invalidRequest.setParams(new ModifiableSolrParams().add("invalid_param", "invalid_value"));
invalidRequest.getParams().add("invalid_param", "invalid_value");

ConsumerRecord<String, MirroredSolrRequest<?>> record =
new ConsumerRecord<>("test-topic", 0, 0, "key", new MirroredSolrRequest<>(invalidRequest));
Expand Down
Loading
Loading