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.

Sign up for GitHub

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

SOLR-17187: make replica poll interval configurable #3144

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+84 −22
Open

SOLR-17187: make replica poll interval configurable #3144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4419d28
Add possibility to supply a custom commitPollInterval
tboeghk Feb 22, 2024
fa9f4bc
[#2316] Move custom commit declaration into `UpdateHandlerInfo`
tboeghk Sep 10, 2024
2070049
Reorder imports
tboeghk Sep 11, 2024
3a6f77f
Reorder assignments by type
tboeghk Sep 11, 2024
a5cd579
Fix format
tboeghk Sep 11, 2024
07f8767
[#2316] [SOLR-17187] Fail on broken poll interval
tboeghk Sep 11, 2024
f5cc010
[#2316] [SOLR-17187] Fix indentation
tboeghk Sep 11, 2024
f659ded
[#2316] [SOLR-17187] Test the poll interval test mode
tboeghk Sep 12, 2024
89f48ff
Merge remote-tracking branch 'origin/main' into PR-2316-continued
cpoerschke Jan 29, 2025
2a547d4
commits-transaction-logs.adoc update
cpoerschke Jan 29, 2025
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
28 changes: 15 additions & 13 deletions solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,9 @@ public void startReplication(boolean switchTransactionLog) {
"SolrCore not found:" + coreName + " in " + CloudUtil.getLoadedCoreNamesAsString(cc));
}
}
SolrConfig.UpdateHandlerInfo uinfo = core.getSolrConfig().getUpdateHandlerInfo();
String pollIntervalStr = "00:00:03";
if (System.getProperty("jetty.testMode") != null) {
pollIntervalStr = "00:00:01";
}

String calculatedPollIntervalString = determinePollInterval(uinfo);
if (calculatedPollIntervalString != null) {
pollIntervalStr = calculatedPollIntervalString;
}
final SolrConfig.UpdateHandlerInfo uinfo = core.getSolrConfig().getUpdateHandlerInfo();
final String pollIntervalStr = determinePollInterval(uinfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is nicer.

log.info("Will start replication from leader with poll interval: {}", pollIntervalStr);

NamedList<Object> followerConfig = new NamedList<>();
Expand Down Expand Up @@ -134,18 +127,25 @@ public static String getCommitVersion(SolrCore solrCore) {
}

/**
* Determine the poll interval for replicas based on the auto soft/hard commit schedule
* Determine the poll interval for replicas based on the auto soft/hard commit schedule or
* configured commit poll interval
*
* @param uinfo the update handler info containing soft/hard commit configuration
* @return a poll interval string representing a cadence of polling frequency in the form of
* hh:mm:ss
* hh:mm:ss, never <code>null</code>
*/
public static String determinePollInterval(SolrConfig.UpdateHandlerInfo uinfo) {
int hardCommitMaxTime = uinfo.autoCommmitMaxTime;
int softCommitMaxTime = uinfo.autoSoftCommmitMaxTime;
boolean hardCommitNewSearcher = uinfo.openSearcher;
String pollIntervalStr = null;
if (hardCommitMaxTime != -1) {
String customCommitPollInterval = uinfo.commitPollInterval;
Copy link
Contributor

Choose a reason for hiding this comment

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

with the comment about the never null, do we need some sort of assert in here?

String pollIntervalStr = "00:00:03";

if (System.getProperty("jetty.testMode") != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we still doing System.getProperty? I thought we had something else that abstracted this away? So you could use a system property or a config file? Maybe my imagination!

Copy link
Contributor

Choose a reason for hiding this comment

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

You are thinking of EnvUtils but that's for real/operational settings a user might set. That doesn't apply here.

pollIntervalStr = "00:00:01";
} else if (customCommitPollInterval != null) {
pollIntervalStr = customCommitPollInterval;
} else if (hardCommitMaxTime != -1) {
// configured hardCommit places a ceiling on the interval at which new segments will be
// available to replicate
if (softCommitMaxTime != -1
Expand All @@ -168,6 +168,8 @@ public static String determinePollInterval(SolrConfig.UpdateHandlerInfo uinfo) {
pollIntervalStr = toPollIntervalStr(softCommitMaxTime / 2);
}

// validate poll interval and fail early
ReplicationHandler.readIntervalNs(pollIntervalStr);
return pollIntervalStr;
}

Expand Down
7 changes: 6 additions & 1 deletion solr/core/src/java/org/apache/solr/core/SolrConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,7 @@ public static class UpdateHandlerInfo implements MapSerializable {
public final long autoCommitMaxSizeBytes;
public final boolean openSearcher; // is opening a new searcher part of hard autocommit?
public final boolean commitWithinSoftCommit;
public final String commitPollInterval;
public final boolean aggregateNodeLevelMetricsEnabled;

/**
Expand All @@ -818,7 +819,8 @@ public UpdateHandlerInfo(
boolean openSearcher,
int autoSoftCommmitMaxDocs,
int autoSoftCommmitMaxTime,
boolean commitWithinSoftCommit) {
boolean commitWithinSoftCommit,
String commitPollInterval) {
this.className = className;
this.autoCommmitMaxDocs = autoCommmitMaxDocs;
this.autoCommmitMaxTime = autoCommmitMaxTime;
Expand All @@ -829,6 +831,7 @@ public UpdateHandlerInfo(
this.autoSoftCommmitMaxTime = autoSoftCommmitMaxTime;

this.commitWithinSoftCommit = commitWithinSoftCommit;
this.commitPollInterval = commitPollInterval;
this.aggregateNodeLevelMetricsEnabled = false;
}

Expand All @@ -844,6 +847,7 @@ public UpdateHandlerInfo(ConfigNode updateHandler) {
this.autoSoftCommmitMaxTime = updateHandler.get("autoSoftCommit").get("maxTime").intVal(-1);
this.commitWithinSoftCommit =
updateHandler.get("commitWithin").get("softCommit").boolVal(true);
this.commitPollInterval = updateHandler.get("commitPollInterval").txt();
this.aggregateNodeLevelMetricsEnabled =
updateHandler.boolAttr("aggregateNodeLevelMetricsEnabled", false);
}
Expand All @@ -860,6 +864,7 @@ public Map<String, Object> toMap(Map<String, Object> map) {
map.put(
"autoSoftCommit",
Map.of("maxDocs", autoSoftCommmitMaxDocs, "maxTime", autoSoftCommmitMaxTime));
map.put("commitPollInterval", commitPollInterval);
return map;
}
}
Expand Down
4 changes: 3 additions & 1 deletion solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.apache.solr.handler.admin.api.ReplicationAPIBase.STATUS;
import static org.apache.solr.handler.admin.api.ReplicationAPIBase.TLOG_FILE;

import com.google.common.annotations.VisibleForTesting;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -1485,7 +1486,8 @@ private Long readIntervalMs(String interval) {
return TimeUnit.MILLISECONDS.convert(readIntervalNs(interval), TimeUnit.NANOSECONDS);
}

private Long readIntervalNs(String interval) {
@VisibleForTesting
public static Long readIntervalNs(String interval) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need the public if you have @VisibleForTesting, or is that just to remind us why it's public?

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed; it explains why it's more open than it otherwise needs to be

if (interval == null) return null;
int result = 0;
Matcher m = INTERVAL_PATTERN.matcher(interval.trim());
Expand Down
65 changes: 58 additions & 7 deletions solr/core/src/test/org/apache/solr/cloud/ReplicateFromLeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,54 +18,105 @@
package org.apache.solr.cloud;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThrows;

import org.apache.solr.common.SolrException;
import org.apache.solr.core.SolrConfig;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

public class ReplicateFromLeaderTest {

private String jettyTestMode;

@Before
public void setUp() {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we have some sort of Rule thing these days for this? @dsmiley ???

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed we do. Firstly, all Solr tests ought to extend SolrTestCase, at least indirectly. This test doesn't; it should be modified. Secondly, STCJ4 includes:

  @Rule public TestRule solrTestRules = RuleChain.outerRule(new SystemPropertiesRestoreRule());

That obsoletes the setup & tearDown here.

So for now, I suggest removing the setup & teardown here and subclass STCJ4. It's a TODO for me in my work on making SolrTestCase a better base class to transition that line from STCJ4 to STC.

jettyTestMode = System.getProperty("jetty.testMode");
}

@After
public void tearDown() {
if (jettyTestMode != null) {
System.setProperty("jetty.testMode", jettyTestMode);
}
}

@Test
public void determineTestPollIntervalString() {
SolrConfig.UpdateHandlerInfo updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 60000, -1, true, -1, -1, false, "0:0:56");
String pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("00:00:01", pollInterval);
}

@Test
public void determinePollIntervalString() {
// disable jetty test mode
System.clearProperty("jetty.testMode");

SolrConfig.UpdateHandlerInfo updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 15000, -1, true, -1, 60000, false);
"solr.DirectUpdateHandler2", -1, 15000, -1, true, -1, 60000, false, null);
String pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:7", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 60000, -1, true, -1, 15000, false);
"solr.DirectUpdateHandler2", -1, 60000, -1, true, -1, 15000, false, null);
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:30", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 15000, -1, false, -1, 60000, false);
"solr.DirectUpdateHandler2", -1, 15000, -1, false, -1, 60000, false, null);
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:30", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 60000, -1, false, -1, 15000, false);
"solr.DirectUpdateHandler2", -1, 60000, -1, false, -1, 15000, false, null);
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:30", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, -1, -1, false, -1, 60000, false);
"solr.DirectUpdateHandler2", -1, -1, -1, false, -1, 60000, false, null);
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:30", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 15000, -1, false, -1, -1, false);
"solr.DirectUpdateHandler2", -1, 15000, -1, false, -1, -1, false, null);
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:7", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 60000, -1, true, -1, -1, false);
"solr.DirectUpdateHandler2", -1, 60000, -1, true, -1, -1, false, null);
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:30", pollInterval);

updateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2", -1, 60000, -1, true, -1, -1, false, "0:0:56");
pollInterval = ReplicateFromLeader.determinePollInterval(updateHandlerInfo);
assertEquals("0:0:56", pollInterval);

final SolrConfig.UpdateHandlerInfo illegalUpdateHandlerInfo =
new SolrConfig.UpdateHandlerInfo(
"solr.DirectUpdateHandler2",
-1,
60000,
-1,
true,
-1,
-1,
false,
"garbage-unfortunately");
assertThrows(
SolrException.class,
() -> ReplicateFromLeader.determinePollInterval(illegalUpdateHandlerInfo));
}
}
2 changes: 2 additions & 0 deletions .../solr-ref-guide/modules/configuration-guide/pages/commits-transaction-logs.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ It is recommended that this be set for as long as is reasonable given the applic
A hard commit means that, if a server crashes, Solr will know exactly where your data was stored; a soft commit means that the data is stored, but the location information isn't yet stored.
The tradeoff is that a soft commit gives you faster visibility because it's not waiting for background merges to finish.

In a TLOG/PULL replica setup, the commit configuration also influences the interval at which the replica is polling the shard leader. You may optionally configure a custom `commitPollInterval`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please start new sentences on new lines. Not sure if this style choice is stated somewhere. Goal is reducing diffs and reducing need for line wrap.


=== Explicit Commits

When a client includes a `commit=true` parameter with an update request, this ensures that all index segments affected by the adds and deletes on an update are written to disk as soon as index updates are completed.
Expand Down
Loading