SOLR-17187: make replica poll interval configurable #3144

cpoerschke · 2025-01-29T16:45:11Z

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).

merged in latest origin/main
added (minimal) Solr Ref Guide update
added solr/CHANGES.txt entry

https://issues.apache.org/jira/browse/SOLR-17187

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]>

- Move poll interval determination into single method

Resolved Conflicts: solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

epugh

Thanks for picking up this PR that has languished for a long time!

epugh · 2025-01-29T17:52:06Z

solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java

-        pollIntervalStr = calculatedPollIntervalString;
-      }
+      final SolrConfig.UpdateHandlerInfo uinfo = core.getSolrConfig().getUpdateHandlerInfo();
+      final String pollIntervalStr = determinePollInterval(uinfo);


this is nicer.

epugh · 2025-01-29T17:52:55Z

solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java

   */
  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;


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

I've added an assert right before this method returns; think that should be sufficient?

solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java

epugh · 2025-01-29T17:56:22Z

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

@@ -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) {


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

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

[Q] Is the @VisibleForTesting explanation accurate though? Maybe we do call it in our tests, but we also call it in ReplicateFromLeader

Unless there's something I'm missing we should probably nuke the annotation.

epugh · 2025-01-29T17:56:54Z

solr/core/src/test/org/apache/solr/cloud/ReplicateFromLeaderTest.java

+  private String jettyTestMode;
+
+  @Before
+  public void setUp() {


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

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.

dsmiley · 2025-02-01T20:39:32Z

solr/solr-ref-guide/modules/configuration-guide/pages/commits-transaction-logs.adoc

@@ -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`.


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.

dsmiley · 2025-02-01T20:42:28Z

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

@@ -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) {


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

dsmiley · 2025-02-01T20:49:19Z

solr/core/src/test/org/apache/solr/cloud/ReplicateFromLeaderTest.java

+  private String jettyTestMode;
+
+  @Before
+  public void setUp() {


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.

github-actions · 2025-04-03T00:00:28Z

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

epugh · 2025-04-03T09:43:13Z

Looks like most of the work has been done.... @cpoerschke do you want to take this over the finish line, or I'm happy to...

cpoerschke · 2025-04-11T11:27:32Z

Looks like most of the work has been done.... @cpoerschke do you want to take this over the finish line, or I'm happy to...

Thanks @epugh! Yes, please feel free to continue with this.

gerlowskija

Took the liberty of tackling the last few TODOs here. If either of you two (@cpoerschke , @epugh ) want to take this back over, go for it. Otherwise I'll aim to commit in the next few days.

Thanks for getting the ball rolling on this @tboeghk - apologies it's taken a bit to get merged.

gerlowskija · 2025-05-13T19:18:00Z

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

@@ -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) {


[Q] Is the @VisibleForTesting explanation accurate though? Maybe we do call it in our tests, but we also call it in ReplicateFromLeader

Unless there's something I'm missing we should probably nuke the annotation.

gerlowskija · 2025-05-13T19:23:18Z

solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java

   */
  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;


I've added an assert right before this method returns; think that should be sufficient?

gerlowskija · 2025-05-13T19:33:06Z

solr/solr-ref-guide/modules/configuration-guide/pages/commits-transaction-logs.adoc

@@ -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`.


gerlowskija · 2025-05-13T20:22:18Z

(Tests, check, and precommit pass locally. I'll add a CHANGES.txt entry right before merge.)

A supplied `commitPollInterval` in `updateHandler` now takes precedence over the default (which is calculated based on hard/soft commit settings). --------- Co-authored-by: Torsten Bøgh Köster <[email protected]> Co-authored-by: Torsten Bøgh Köster <[email protected]> Co-authored-by: Christine Poerschke <[email protected]> Co-authored-by: Jason Gerlowski <[email protected]>

tboeghk and others added 10 commits February 22, 2024 13:53

Add possibility to supply a custom commitPollInterval

4419d28

A supplied `commitPollInterval` in `updateHandler` this has precendence over one calculated from hard/soft commit settings

[apache#2316] Move custom commit declaration into UpdateHandlerInfo

fa9f4bc

Also add tests for retrieving and selecting the commit interval

Reorder imports

2070049

Co-authored-by: Christine Poerschke <[email protected]>

Reorder assignments by type

3a6f77f

Co-authored-by: Christine Poerschke <[email protected]>

Fix format

a5cd579

Co-authored-by: Christine Poerschke <[email protected]>

[apache#2316] [SOLR-17187] Fail on broken poll interval

07f8767

- Move poll interval determination into single method

[apache#2316] [SOLR-17187] Fix indentation

f5cc010

[apache#2316] [SOLR-17187] Test the poll interval test mode

f659ded

Merge remote-tracking branch 'origin/main' into PR-2316-continued

89f48ff

Resolved Conflicts: solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

commits-transaction-logs.adoc update

2a547d4

github-actions bot added documentation Improvements or additions to documentation tests cat:cloud labels Jan 29, 2025

cpoerschke mentioned this pull request Jan 29, 2025

[SOLR-17187] Add possibility to supply a custom commitPollInterval for TLOG/PULL replicas #2316

Closed

7 tasks

cpoerschke marked this pull request as ready for review January 29, 2025 17:01

epugh reviewed Jan 29, 2025

View reviewed changes

dsmiley reviewed Feb 1, 2025

View reviewed changes

github-actions bot added the stale PR not updated in 60 days label Apr 3, 2025

github-actions bot removed the stale PR not updated in 60 days label Apr 4, 2025

gerlowskija added 3 commits May 13, 2025 15:40

Address review comments, rd 1

3a3d0a9

Merge branch 'main' into PR-2316-continued

8a5d072

Fix check

15d7b32

gerlowskija approved these changes May 13, 2025

View reviewed changes

gerlowskija added 2 commits May 16, 2025 09:50

Merge branch 'main' into PR-2316-continued

e29cd66

CHANGES.txt entry

fd7caea

gerlowskija merged commit 2b1fbc1 into apache:main May 16, 2025
1 of 3 checks passed

SOLR-17187: make replica poll interval configurable #3144

SOLR-17187: make replica poll interval configurable #3144

Uh oh!

Conversation

cpoerschke commented Jan 29, 2025

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

epugh commented Apr 3, 2025

Uh oh!

cpoerschke commented Apr 11, 2025

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerlowskija commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!