epugh commented on code in PR #3144:
URL: https://github.com/apache/solr/pull/3144#discussion_r1934338854
##########
solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java:
##########
@@ -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);
Review Comment:
this is nicer.
##########
solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java:
##########
@@ -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;
Review Comment:
with the comment about the never null, do we need some sort of `assert` in
here?
##########
solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java:
##########
@@ -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;
+ String pollIntervalStr = "00:00:03";
+
+ if (System.getProperty("jetty.testMode") != null) {
Review 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!
##########
solr/core/src/test/org/apache/solr/cloud/ReplicateFromLeaderTest.java:
##########
@@ -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() {
Review Comment:
don't we have some sort of Rule thing these days for this? @dsmiley ???
##########
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) {
Review Comment:
do you need the `public` if you have `@VisibleForTesting`, or is that just
to remind us why it's public?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
