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/17 21:52:28 UTC
[GitHub] [lucene-solr] s1monw opened a new pull request #1590: LUCENE-9408: Ensure OneMerge#mergeFinished is only called once
s1monw opened a new pull request #1590:
URL: https://github.com/apache/lucene-solr/pull/1590
in the case of an exception it's possible that some OneMerge instances
will be closed multiple times. This commit ensures that mergeFinished is
really just called once instead of multiple times.
----------------------------------------------------------------
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 #1590: LUCENE-9408: Ensure OneMerge#mergeFinished is only called once
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1590:
URL: https://github.com/apache/lucene-solr/pull/1590#discussion_r445010346
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -4286,25 +4287,30 @@ 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(suppressExceptions == false)) {
- IOUtils.applyToAll(merge.readers, sr -> {
- final ReadersAndUpdates rld = getPooledInstance(sr.getOriginalSegmentInfo(), false);
- // We still hold a ref so it should not have been removed:
- assert rld != null;
- if (drop) {
- rld.dropChanges();
- } else {
- rld.dropMergingUpdates();
- }
- rld.release(sr);
- release(rld);
- if (drop) {
- readerPool.drop(rld.info);
- }
- });
- } finally {
- Collections.fill(merge.readers, null);
+ if (merge.isDone() == false) {
Review comment:
@mikemccand I pushed a new commit to address this
----------------------------------------------------------------
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 #1590: LUCENE-9408: Ensure OneMerge#mergeFinished is only called once
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1590:
URL: https://github.com/apache/lucene-solr/pull/1590#discussion_r443538263
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -4286,25 +4287,30 @@ 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(suppressExceptions == false)) {
- IOUtils.applyToAll(merge.readers, sr -> {
- final ReadersAndUpdates rld = getPooledInstance(sr.getOriginalSegmentInfo(), false);
- // We still hold a ref so it should not have been removed:
- assert rld != null;
- if (drop) {
- rld.dropChanges();
- } else {
- rld.dropMergingUpdates();
- }
- rld.release(sr);
- release(rld);
- if (drop) {
- readerPool.drop(rld.info);
- }
- });
- } finally {
- Collections.fill(merge.readers, null);
+ if (merge.isDone() == false) {
Review comment:
It is a little weird that we don't then have a `merge.setDone()` or something inside this `if` to make the idempotency really clear. But I understand this code is pre-existingly already hairy so it is not simple to fix that, so progress not perfection!
----------------------------------------------------------------
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 #1590: LUCENE-9408: Ensure OneMerge#mergeFinished is only called once
Posted by GitBox <gi...@apache.org>.
s1monw merged pull request #1590:
URL: https://github.com/apache/lucene-solr/pull/1590
----------------------------------------------------------------
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 #1590: LUCENE-9408: Ensure OneMerge#mergeFinished is only called once
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1590:
URL: https://github.com/apache/lucene-solr/pull/1590#discussion_r443728610
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -4286,25 +4287,30 @@ 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(suppressExceptions == false)) {
- IOUtils.applyToAll(merge.readers, sr -> {
- final ReadersAndUpdates rld = getPooledInstance(sr.getOriginalSegmentInfo(), false);
- // We still hold a ref so it should not have been removed:
- assert rld != null;
- if (drop) {
- rld.dropChanges();
- } else {
- rld.dropMergingUpdates();
- }
- rld.release(sr);
- release(rld);
- if (drop) {
- readerPool.drop(rld.info);
- }
- });
- } finally {
- Collections.fill(merge.readers, null);
+ if (merge.isDone() == false) {
Review comment:
I can call it `hasFinished()` that would make it more clear? WDYT
----------------------------------------------------------------
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