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/16 20:23:32 UTC
[GitHub] [lucene-solr] s1monw opened a new pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
s1monw opened a new pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585
This change adds infrastructure to allow straight forward waiting
on one or more merges or an entire merge specification. This is
a basis for LUCENE-8962.
----------------------------------------------------------------
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] s1monw commented on pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
s1monw commented on pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#issuecomment-644992788
@mikemccand @msokolov @msfroh I prepared some of my changes so we can pull them in and incorporate them into the overall change.
----------------------------------------------------------------
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] msokolov commented on a change in pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
msokolov commented on a change in pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#discussion_r441170320
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -4289,7 +4287,7 @@ private synchronized void mergeFinish(MergePolicy.OneMerge merge) {
@SuppressWarnings("try")
private synchronized void closeMergeReaders(MergePolicy.OneMerge merge, boolean suppressExceptions) throws IOException {
final boolean drop = suppressExceptions == false;
- try (Closeable finalizer = merge::mergeFinished) {
+ try (Closeable finalizer = () -> merge.mergeFinished(suppressExceptions==false)) {
Review comment:
how about some spaces around the operator==?
----------------------------------------------------------------
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] s1monw commented on a change in pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#discussion_r441575828
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -196,6 +200,7 @@ final void setMergeThread(Thread owner) {
*
* @lucene.experimental */
public static class OneMerge {
Review comment:
yeah I want to do that anyway
----------------------------------------------------------------
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] msfroh commented on a change in pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
msfroh commented on a change in pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#discussion_r441148085
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -399,6 +427,23 @@ public String segString(Directory dir) {
}
return b.toString();
}
+
+ /**
+ * Waits if necessary for at most the given time for all merges.
+ */
+ boolean await(long timeout, TimeUnit unit) {
+ try {
+ CompletableFuture<Void> future = CompletableFuture.allOf(merges.stream()
+ .map(m -> m.completable).collect(Collectors.toList()).toArray(new CompletableFuture<?>[0]));
+ future.get(timeout, unit);
+ return true;
+ } catch (InterruptedException e) {
+ Thread.interrupted();
Review comment:
In my change, I remember I originally tried handling the `InterruptedException` while waiting for the `CountDownLatch` in a similar way, but it caused an intermittent test failure (in `TestIndexWriterWithThreads`, maybe?) You might run into the same test failure once you start calling this `await` method from within `IndexWriter`.
I was able to get the test passing by rethrowing it wrapped in a `ThreadInterruptedException`.
----------------------------------------------------------------
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] mikemccand commented on a change in pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#discussion_r441559509
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -196,6 +200,7 @@ final void setMergeThread(Thread owner) {
*
* @lucene.experimental */
public static class OneMerge {
+ private final CompletableFuture<Boolean> completable = new CompletableFuture<>();
Review comment:
Could we rename the variable to `completed` or maybe `mergeCompleted`?
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -196,6 +200,7 @@ final void setMergeThread(Thread owner) {
*
* @lucene.experimental */
public static class OneMerge {
Review comment:
Maybe we should (later, in dedicated PR) pull this out into its own java source?
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -362,6 +366,30 @@ public void checkAborted() throws MergeAbortedException {
public OneMergeProgress getMergeProgress() {
return mergeProgress;
}
+
+ /**
+ * Waits for this merge to be completed
+ * @return true if the merge finished within the specified timeout
+ */
+ boolean await(long timeout, TimeUnit timeUnit) {
+ try {
+ completable.get(timeout, timeUnit);
+ return true;
+ } catch (InterruptedException e) {
+ Thread.interrupted();
+ return false;
+ } catch (ExecutionException | TimeoutException e) {
+ return false;
+ }
+ }
+
+ boolean isDone() {
Review comment:
Maybe add javadoc about the lack of thread safety here? I.e. this could return `false` and shortly thereafter it becomes `true`.
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -362,6 +366,30 @@ public void checkAborted() throws MergeAbortedException {
public OneMergeProgress getMergeProgress() {
return mergeProgress;
}
+
+ /**
+ * Waits for this merge to be completed
+ * @return true if the merge finished within the specified timeout
+ */
+ boolean await(long timeout, TimeUnit timeUnit) {
+ try {
+ completable.get(timeout, timeUnit);
+ return true;
+ } catch (InterruptedException e) {
+ Thread.interrupted();
+ return false;
+ } catch (ExecutionException | TimeoutException e) {
+ return false;
+ }
+ }
+
+ boolean isDone() {
+ return completable.isDone();
+ }
+
+ boolean isCommitted() {
Review comment:
Maybe rename to `isCompleted`? `committed` is overloaded term -- could try to mean its files got `fsync'`d :)
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -362,6 +366,30 @@ public void checkAborted() throws MergeAbortedException {
public OneMergeProgress getMergeProgress() {
return mergeProgress;
}
+
+ /**
+ * Waits for this merge to be completed
+ * @return true if the merge finished within the specified timeout
+ */
+ boolean await(long timeout, TimeUnit timeUnit) {
+ try {
+ completable.get(timeout, timeUnit);
+ return true;
+ } catch (InterruptedException e) {
+ Thread.interrupted();
+ return false;
+ } catch (ExecutionException | TimeoutException e) {
+ return false;
+ }
+ }
+
+ boolean isDone() {
+ return completable.isDone();
+ }
+
+ boolean isCommitted() {
+ return completable.getNow(Boolean.FALSE);
Review comment:
Javadoc here too?
----------------------------------------------------------------
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] s1monw commented on a change in pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#discussion_r441581487
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -4289,7 +4287,7 @@ private synchronized void mergeFinish(MergePolicy.OneMerge merge) {
@SuppressWarnings("try")
private synchronized void closeMergeReaders(MergePolicy.OneMerge merge, boolean suppressExceptions) throws IOException {
final boolean drop = suppressExceptions == false;
- try (Closeable finalizer = merge::mergeFinished) {
+ try (Closeable finalizer = () -> merge.mergeFinished(suppressExceptions==false)) {
Review comment:
done
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -399,6 +427,23 @@ public String segString(Directory dir) {
}
return b.toString();
}
+
+ /**
+ * Waits if necessary for at most the given time for all merges.
+ */
+ boolean await(long timeout, TimeUnit unit) {
+ try {
+ CompletableFuture<Void> future = CompletableFuture.allOf(merges.stream()
+ .map(m -> m.completable).collect(Collectors.toList()).toArray(new CompletableFuture<?>[0]));
+ future.get(timeout, unit);
+ return true;
+ } catch (InterruptedException e) {
+ Thread.interrupted();
Review comment:
done
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -399,6 +427,23 @@ public String segString(Directory dir) {
}
return b.toString();
}
+
+ /**
+ * Waits if necessary for at most the given time for all merges.
+ */
+ boolean await(long timeout, TimeUnit unit) {
+ try {
+ CompletableFuture<Void> future = CompletableFuture.allOf(merges.stream()
Review comment:
done thanks
----------------------------------------------------------------
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] s1monw merged pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
s1monw merged pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585
----------------------------------------------------------------
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 #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#discussion_r441140226
##########
File path: lucene/core/src/java/org/apache/lucene/index/MergePolicy.java
##########
@@ -399,6 +427,23 @@ public String segString(Directory dir) {
}
return b.toString();
}
+
+ /**
+ * Waits if necessary for at most the given time for all merges.
+ */
+ boolean await(long timeout, TimeUnit unit) {
+ try {
+ CompletableFuture<Void> future = CompletableFuture.allOf(merges.stream()
Review comment:
What I learned today: `CompletableFuture.allOf(....)` -- cool, thanks! So elegant.
Let me try and return the favor:
You can go directly to an array, avoiding toList:
```
CompletableFuture<Void> future = CompletableFuture.allOf(merges.stream()
.map(m -> m.completable).toArray(CompletableFuture<?>[]::new));
```
BTW IntelliJ pointed this out and had an automatic replacement when I hovered my cursor over "collect".
----------------------------------------------------------------
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] s1monw commented on pull request #1585: LUCENE-8962: Allow waiting for all merges in a merge spec
Posted by GitBox <gi...@apache.org>.
s1monw commented on pull request #1585:
URL: https://github.com/apache/lucene-solr/pull/1585#issuecomment-645403443
@mikemccand @msokolov @msfroh @dsmiley I pushed a new commit to address your comments. thanks you!
----------------------------------------------------------------
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