You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/06/22 22:33:58 UTC
[GitHub] [lucene-solr] tflobbe opened a new pull request #1602: Expose IWC.setMaxCommitMergeWaitSeconds in Solr's index config
tflobbe opened a new pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602
LUCENE-8962 added the ability to merge segments synchronously on commit. This isn't done by default and the default MergePolicy won't do it, but custom merge policies can take advantage of this. Solr allows plugging in custom merge policies, so if someone wants to make use of this feature they could, however, they need to set IndexWriterConfig.maxCommitMergeWaitSeconds to something greater than 0.
Since this is an expert feature, I plan to document it only in javadoc and not the ref guide.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466526966
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -87,6 +100,7 @@ private SolrIndexConfig(SolrConfig solrConfig) {
maxBufferedDocs = -1;
ramBufferSizeMB = 100;
ramPerThreadHardLimitMB = -1;
+ maxCommitMergeWaitMillis = -1;
Review comment:
Lets use `org.apache.lucene.index.IndexWriterConfig#DEFAULT_MAX_COMMIT_MERGE_WAIT_MILLIS`. In the future, this constant might change, and that's good.
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -68,6 +68,19 @@
public final double ramBufferSizeMB;
public final int ramPerThreadHardLimitMB;
+ /**
Review comment:
I appreciate documentation but _redundant_ documentation -- not so much. Can't you do a @see or @link to one place -- `org.apache.lucene.index.IndexWriterConfig#setMaxCommitMergeWaitMillis`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466565653
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -129,8 +143,9 @@ public SolrIndexConfig(SolrConfig solrConfig, String prefix, SolrIndexConfig def
true);
useCompoundFile = solrConfig.getBool(prefix+"/useCompoundFile", def.useCompoundFile);
- maxBufferedDocs=solrConfig.getInt(prefix+"/maxBufferedDocs",def.maxBufferedDocs);
+ maxBufferedDocs = solrConfig.getInt(prefix+"/maxBufferedDocs", def.maxBufferedDocs);
ramBufferSizeMB = solrConfig.getDouble(prefix+"/ramBufferSizeMB", def.ramBufferSizeMB);
+ maxCommitMergeWaitMillis = solrConfig.getInt(prefix+"/maxCommitMergeWait", def.maxCommitMergeWaitMillis);
Review comment:
I had it with `MIllis` and I removed that because I didn't see anything else in the solrconfig that included the unit for time (it's always milliseconds). You are suggesting `maxCommitMergeWait` or `maxCommitMergeTime`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466561784
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -87,6 +100,7 @@ private SolrIndexConfig(SolrConfig solrConfig) {
maxBufferedDocs = -1;
ramBufferSizeMB = 100;
ramPerThreadHardLimitMB = -1;
+ maxCommitMergeWaitMillis = -1;
Review comment:
I don't think that's a good idea. "-1" from the Solr perspective means "don't set the value". If someone has a custom merge policy where they have a different value set by code, we would be changing it to `org.apache.lucene.index.IndexWriterConfig#DEFAULT_MAX_COMMIT_MERGE_WAIT_MILLIS` without them having set anything on `solrconfig.xml`.
> In the future, this constant might change, and that's good.
That's fine, since we aren't calling `iwc.setMaxCommitMergeWaitMillis(...)` in the default case, we'll be taking the new default for all cores where no alternative configuration has been provided.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466530152
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -129,8 +143,9 @@ public SolrIndexConfig(SolrConfig solrConfig, String prefix, SolrIndexConfig def
true);
useCompoundFile = solrConfig.getBool(prefix+"/useCompoundFile", def.useCompoundFile);
- maxBufferedDocs=solrConfig.getInt(prefix+"/maxBufferedDocs",def.maxBufferedDocs);
+ maxBufferedDocs = solrConfig.getInt(prefix+"/maxBufferedDocs", def.maxBufferedDocs);
ramBufferSizeMB = solrConfig.getDouble(prefix+"/ramBufferSizeMB", def.ramBufferSizeMB);
+ maxCommitMergeWaitMillis = solrConfig.getInt(prefix+"/maxCommitMergeWait", def.maxCommitMergeWaitMillis);
Review comment:
I noticed you omitted a "Millis" setting. I think either you should add this for clarity, or for consistency with some other times I see in solrconfig (e.g. maxTime), use suffix "Time".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466818795
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -87,6 +100,7 @@ private SolrIndexConfig(SolrConfig solrConfig) {
maxBufferedDocs = -1;
ramBufferSizeMB = 100;
ramPerThreadHardLimitMB = -1;
+ maxCommitMergeWaitMillis = -1;
Review comment:
ok, good point.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466589294
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -208,4 +210,16 @@ public void testToMap() throws Exception {
assertEquals(mSizeExpected, m.size());
}
+
+ public void testMaxCommitMergeWaitSeconds() throws Exception {
+ SolrConfig sc = new SolrConfig(TEST_PATH().resolve("collection1"), "solrconfig-test-misc.xml");
+ assertEquals(-1, sc.indexConfig.maxCommitMergeWaitMillis);
+ assertEquals(IndexWriterConfig.DEFAULT_MAX_COMMIT_MERGE_WAIT_MILLIS, sc.indexConfig.toIndexWriterConfig(h.getCore()).getMaxCommitMergeWaitMillis());
+ System.setProperty("solr.tests.maxCommitMergeWait", "10");
+ sc = new SolrConfig(TEST_PATH().resolve("collection1"), "solrconfig-test-misc.xml");
+ assertEquals(10, sc.indexConfig.maxCommitMergeWaitMillis);
+ assertEquals(10, sc.indexConfig.toIndexWriterConfig(h.getCore()).getMaxCommitMergeWaitMillis());
+ System.clearProperty("solr.tests.maxCommitMergeWait");
Review comment:
Please verify if this is so before removing. Perhaps there may be a difference in behavior between gradle & IntelliJ; so try both. I thought clearing wasn't necessary as well but I recall hearing from someone (@dweiss ?) that the auto-clearing wasn't working.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe merged pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe merged pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466587705
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -129,8 +143,9 @@ public SolrIndexConfig(SolrConfig solrConfig, String prefix, SolrIndexConfig def
true);
useCompoundFile = solrConfig.getBool(prefix+"/useCompoundFile", def.useCompoundFile);
- maxBufferedDocs=solrConfig.getInt(prefix+"/maxBufferedDocs",def.maxBufferedDocs);
+ maxBufferedDocs = solrConfig.getInt(prefix+"/maxBufferedDocs", def.maxBufferedDocs);
ramBufferSizeMB = solrConfig.getDouble(prefix+"/ramBufferSizeMB", def.ramBufferSizeMB);
+ maxCommitMergeWaitMillis = solrConfig.getInt(prefix+"/maxCommitMergeWait", def.maxCommitMergeWaitMillis);
Review comment:
The "Wait" part is important too too. "maxCommitMergeWaitTime" is my preference
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466565653
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -129,8 +143,9 @@ public SolrIndexConfig(SolrConfig solrConfig, String prefix, SolrIndexConfig def
true);
useCompoundFile = solrConfig.getBool(prefix+"/useCompoundFile", def.useCompoundFile);
- maxBufferedDocs=solrConfig.getInt(prefix+"/maxBufferedDocs",def.maxBufferedDocs);
+ maxBufferedDocs = solrConfig.getInt(prefix+"/maxBufferedDocs", def.maxBufferedDocs);
ramBufferSizeMB = solrConfig.getDouble(prefix+"/ramBufferSizeMB", def.ramBufferSizeMB);
+ maxCommitMergeWaitMillis = solrConfig.getInt(prefix+"/maxCommitMergeWait", def.maxCommitMergeWaitMillis);
Review comment:
I had it with `MIllis` and I removed that because I didn't see anything else in the solrconfig that included the unit for time (it's always milliseconds). You are suggesting `maxCommitMergeWaitMillis` or `maxCommitMergeTime`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466603545
##########
File path: solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
##########
@@ -208,4 +210,16 @@ public void testToMap() throws Exception {
assertEquals(mSizeExpected, m.size());
}
+
+ public void testMaxCommitMergeWaitSeconds() throws Exception {
+ SolrConfig sc = new SolrConfig(TEST_PATH().resolve("collection1"), "solrconfig-test-misc.xml");
+ assertEquals(-1, sc.indexConfig.maxCommitMergeWaitMillis);
+ assertEquals(IndexWriterConfig.DEFAULT_MAX_COMMIT_MERGE_WAIT_MILLIS, sc.indexConfig.toIndexWriterConfig(h.getCore()).getMaxCommitMergeWaitMillis());
+ System.setProperty("solr.tests.maxCommitMergeWait", "10");
+ sc = new SolrConfig(TEST_PATH().resolve("collection1"), "solrconfig-test-misc.xml");
+ assertEquals(10, sc.indexConfig.maxCommitMergeWaitMillis);
+ assertEquals(10, sc.indexConfig.toIndexWriterConfig(h.getCore()).getMaxCommitMergeWaitMillis());
+ System.clearProperty("solr.tests.maxCommitMergeWait");
Review comment:
Yes, I checked. I believe the cleanup is after the class. I discussed with @madrob, and I'll move the cleanup to a `tearDown()` method
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe commented on pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#issuecomment-670593930
@dsmiley, looks good? should I merge?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gus-asf commented on pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#issuecomment-670739812
I think this breaks the build... I'm seeing a failure in org.apache.solr.core.TestConfig#testDefaults which doesn't seem to account for maxCommitMergeWaitTime also lots of failures on http://fucit.org/solr-jenkins-reports/failure-report.html in last 24h for this test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1602: SOLR-14582: Expose IWC.setMaxCommitMergeWaitMillis in Solr's index config
Posted by GitBox <gi...@apache.org>.
tflobbe commented on a change in pull request #1602:
URL: https://github.com/apache/lucene-solr/pull/1602#discussion_r466606166
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -68,6 +68,19 @@
public final double ramBufferSizeMB;
public final int ramPerThreadHardLimitMB;
+ /**
Review comment:
I didn't want to drop the link/see by itself, so I added a single line introducing the functionality. After that I added specifics about Solr (solrconfig configuration and the warning setting). I honestly don't see the problem here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org