rmuir commented on a change in pull request #128:
URL: https://github.com/apache/lucene/pull/128#discussion_r628122895
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -488,8 +503,35 @@ public Status checkIndex() throws IOException {
* quite a long time to run.
*/
public Status checkIndex(List<String> onlySegments) throws IOException {
Review comment:
please don't overload it here. force callers to pass executor.
I am still worried that tests are using multiple threads here which must not
happen: on an 8 core machine we don't want to use 8 jvms * 8 threads each.
I am also concerned about newly-created synchronization issues here (e.g.
with output). If checkindex fails it is really important that we can read this
output.
All these changes leave my concerned that this is the right way to go. At
the minimal, if we are going to make these changes to CheckIndex, then it needs
to be done in a more cautious/defensive way (e.g. default threadcount to 1
instead of numcpus).
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
