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

Jump to bottom

OAK-11443 - Use a single DefaultIndexWriterFactory in the LuceneIndexEditorProvider #2041

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Draft

OAK-11443 - Use a single DefaultIndexWriterFactory in the LuceneIndexEditorProvider #2041

wants to merge 2 commits into from

Conversation

nfsantos
Copy link
Contributor

No description provided.

thomasmueller
thomasmueller reviewed Feb 12, 2025
View reviewed changes
oak-store-spi/src/main/java/org/apache/jackrabbit/oak/plugins/memory/ModifiedNodeState.java
@@ -165,7 +166,7 @@ static Iterable<? extends PropertyState> getProperties(
x -> !keys.contains(x == null ? null : x.getName());
return concat(
filter(base.getProperties(), predicate::test),
filter(properties.values(), x -> x != null));
filter(properties.values(), Objects::nonNull));
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated...

.../src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexEditorProvider.java
if (defaultIndexWriterFactory == null) {
LOG.info("Initializing DefaultIndexWriterFactory");
cowDirectoryCleanupCallback = new COWDirectoryCleanupCallback();
BlobDeletionCallback blobDeletionCallback = activeDeletedBlobCollector.getBlobDeletionCallback();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it is possible to add a test case to show the problem of the current code

@nfsantos nfsantos marked this pull request as draft February 13, 2025 05:42
@nfsantos
Copy link
Contributor Author

nfsantos commented Feb 13, 2025 •
edited
Loading

On hold until OAK-11438 is merged. The main motivation for this PR was to make it easy for OAK-11438 for close the thread pool IndexWriterPool that would be added inside DefaultIndexWriterFactory. But I have refactored OAK-11438 to move the ownership of the IndexWriterPool from the instances of DefaultIndexWriterFactory to the LuceneIndexWriterProvider that creates them. The provider can be easily closed at the end of the indexing. Therefore, the individual instances of DefaultIndexWriterFactories will no longer own a thread pool, so they can be left for the garbage collector to handle as it is done currently. There would still be an efficiency gain of reusing a single DefaultIndexWriterFactory but the current implementation makes this quite complex and would need a significant refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasmueller thomasmueller thomasmueller left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nfsantos @thomasmueller