Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close old reference manager when using filtering searcher manager #629

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Close old reference manager when using filtering searcher manager #629

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/java/com/yelp/nrtsearch/server/luceneserver/NRTReplicaNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
import org.apache.lucene.replicator.nrt.NodeCommunicationException;
import org.apache.lucene.replicator.nrt.ReplicaDeleterManager;
import org.apache.lucene.replicator.nrt.ReplicaNode;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.ReferenceManager;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.store.Directory;
import org.slf4j.Logger;
Expand Down Expand Up @@ -121,7 +123,9 @@ public synchronized void start(long primaryGen) throws IOException {
// Swap in a SearcherManager that filters incompatible segment readers during refresh.
// Updating the reference is not thread safe, but since this happens under the object lock
// and before the shard has stared, nothing should access the manager before the swap.
ReferenceManager<IndexSearcher> oldMgr = mgr;
mgr = new FilteringSegmentInfosSearcherManager(getDirectory(), this, mgr, searcherFactory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears as if FilteringSegmentInfosSearcherManager is using the old reference manager because it's passed in the constructor, but it only gets the current SegmentInfos: ((SegmentInfosSearcherManager) mgr).getCurrentInfos()
Can we change the constructor to accept SegmentInfos and then pass it from here so that it's clear that the old reference manager is not accessed any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, SegmentInfosSearcherManager is package private and cannot be used from NRTReplicaNode unless we relocate the class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

oldMgr.close();
}
}

Expand Down
Loading