Skip to content

OAK-12054: Refactor creation of ThreadPoolExecutors#2679

Merged
thomasmueller merged 1 commit intoapache:trunkfrom
bhabegger:fix-executor-service
Feb 3, 2026
Merged

OAK-12054: Refactor creation of ThreadPoolExecutors#2679
thomasmueller merged 1 commit intoapache:trunkfrom
bhabegger:fix-executor-service

Conversation

@bhabegger
Copy link
Copy Markdown
Contributor

Some instantiations of ThreadPoolExecutor were setting a different corePoolSize and maxPoolSize even though they were using an unbounded LinkedBlockingQueue. The pattern was repeated multiple times. This PR attempts to fix and mutualize the ThreadPoolExecutor creation code.

Comment thread oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/IndexHelper.java Outdated
Comment thread oak-run/src/main/java/org/apache/jackrabbit/oak/run/Downloader.java
@joerghoh
Copy link
Copy Markdown
Contributor

joerghoh commented Jan 9, 2026

@bhabegger good catch, thanks for the PR; can you create a dedicated OAK ticket for it?

@bhabegger bhabegger force-pushed the fix-executor-service branch from 4318b53 to e61cba0 Compare January 12, 2026 07:57
@bhabegger
Copy link
Copy Markdown
Contributor Author

@bhabegger good catch, thanks for the PR; can you create a dedicated OAK ticket for it?

Well, in fact it was hinted to me by @thomasmueller as a pending task that I took for my onboarding :) So, I don't have much credit for catching this;)

@bhabegger bhabegger force-pushed the fix-executor-service branch from e61cba0 to 33271ab Compare January 12, 2026 08:17
@bhabegger bhabegger changed the title Refactor creation of ThreadPoolExecutors OAK-12054: Refactor creation of ThreadPoolExecutors Jan 12, 2026
@bhabegger bhabegger force-pushed the fix-executor-service branch from 33271ab to eb16e23 Compare January 13, 2026 12:35
@bhabegger bhabegger force-pushed the fix-executor-service branch from eb16e23 to 28c2927 Compare January 15, 2026 07:16
Comment thread oak-auth-ldap/pom.xml
Copy link
Copy Markdown
Member

@thomasmueller thomasmueller left a comment

Choose a reason for hiding this comment

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

There is a risk if we change the current behavior, even if the current behavior is not what was intended originally: we could have out-of-memory, concurrency issues etc.

So either the PR should not change the current behavior, or at least, we should have a way to switch back to the old behavior, in case of issues. E.g. with a feature toggle.

Given that we don't currently see issue with the current behavior (other than it's not intended, and can confuse people, etc.), I think we should keep the current behavior.

@bhabegger bhabegger force-pushed the fix-executor-service branch 3 times, most recently from 687a16b to 443d867 Compare January 16, 2026 07:14
@bhabegger bhabegger requested a review from rishabhdaim January 16, 2026 07:17
Copy link
Copy Markdown
Member

@thomasmueller thomasmueller left a comment

Choose a reason for hiding this comment

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

Just some formatting.

Comment thread oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/IndexHelper.java Outdated
@bhabegger bhabegger force-pushed the fix-executor-service branch from 5f19daa to 60ee9fa Compare January 16, 2026 11:11
@bhabegger bhabegger force-pushed the fix-executor-service branch from 60ee9fa to 3f44d08 Compare January 22, 2026 07:27
@bhabegger bhabegger force-pushed the fix-executor-service branch from 3f44d08 to 2a6f894 Compare January 23, 2026 07:29
}

public static final String REPOSITORY_HOME = "repository.home";
private static final int INDEX_COPIER_POOL_SIZE = 5;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: if we only use it in one place, it seems not useful to have a constant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see 2 benefits though:

  • readability by having a name rather than just a number
  • having the "setup" of the class at the top (these are potential candidate parameters)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(But I won't fight over it :))

@thomasmueller thomasmueller merged commit 0861c05 into apache:trunk Feb 3, 2026
1 of 2 checks passed
reschke pushed a commit that referenced this pull request Feb 4, 2026
reschke pushed a commit that referenced this pull request Feb 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants