SOLR-17187: make replica poll interval configurable #3144
Conversation
A supplied `commitPollInterval` in `updateHandler` this has precendence over one calculated from hard/soft commit settings
Also add tests for retrieving and selecting the commit interval
Co-authored-by: Christine Poerschke <[email protected]>
Co-authored-by: Christine Poerschke <[email protected]>
Co-authored-by: Christine Poerschke <[email protected]>
- Move poll interval determination into single method
Resolved Conflicts: solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java
github-actions
bot
added
documentation
| pollIntervalStr = calculatedPollIntervalString; | ||
| } | ||
| final SolrConfig.UpdateHandlerInfo uinfo = core.getSolrConfig().getUpdateHandlerInfo(); | ||
| final String pollIntervalStr = determinePollInterval(uinfo); |
| */ | ||
| 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; |
There was a problem hiding this comment.
with the comment about the never null, do we need some sort of assert in here?
| String customCommitPollInterval = uinfo.commitPollInterval; | ||
| String pollIntervalStr = "00:00:03"; | ||
|
|
||
| if (System.getProperty("jetty.testMode") != null) { |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
You are thinking of EnvUtils but that's for real/operational settings a user might set. That doesn't apply here.
| @@ -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) { | |||
There was a problem hiding this comment.
do you need the public if you have @VisibleForTesting, or is that just to remind us why it's public?
There was a problem hiding this comment.
indeed; it explains why it's more open than it otherwise needs to be
| private String jettyTestMode; | ||
|
|
||
| @Before | ||
| public void setUp() { |
There was a problem hiding this comment.
don't we have some sort of Rule thing these days for this? @dsmiley ???
There was a problem hiding this comment.
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.
| @@ -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`. | |||
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
indeed; it explains why it's more open than it otherwise needs to be
| private String jettyTestMode; | ||
|
|
||
| @Before | ||
| public void setUp() { |
There was a problem hiding this comment.
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.
Continuation of @tboeghk's #2316 as a new pull request since pushing to the PR's branch is not possible (due to org vs. individual repo fork).
origin/mainhttps://issues.apache.org/jira/browse/SOLR-17187