Skip to content

Conversation

@kirklund
Copy link
Contributor

Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.

Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.
@kirklund kirklund requested a review from mhansonp as a code owner October 14, 2021 16:46
@kirklund kirklund requested a review from Bill October 14, 2021 16:46
Copy link
Contributor

@Bill Bill left a comment

Choose a reason for hiding this comment

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

Thanks for making this happen @kirklund ! Your knowledge of this framework apparently made this short work for you.

I did make a suggestion about paring down the constructors even further, but this is ok as-is if you disagree with that idea.

boolean awaitTerminationBeforeShutdown,
boolean useShutdown,
boolean useShutdownNow,
int threadCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor has a lot of parameters. Especially troublesome (for callers) is that it has three-in-a-row of the same type.

I wonder if this class might be improved by making ExecutorServiceRule(Builder builder) the "most general" constuctor, i.e. having other constructors e.g. ExecutorServiceRule(int threadCount) and ExecutorServiceRule() call that one.

If you chose to do that then this constructor would go away. And we'd have (assuming appropriate defaults in the Builder class) e.g.

public ExecutorServiceRule(int threadCount) {
  this(new Builder().threadCount(threadCount));
}

and

  public ExecutorServiceRule() {
    this(new Builder());
  }

Ooh I just noticed this PR almost does this in DistributedExecutorServiceRule! The one change I'd suggest in that class, as here, would be to simply make the builder-taking constructor the most-general constructor and forego the constructor(s) that take the exploded parameter list.

boolean useShutdown,
boolean useShutdownNow,
int threadCount,
int vmCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor isn't needed at all. See my comments over on ExecutorServiceRule.

Love how this class' public constructors both call on the builder-taking constructor. It makes those calls very readable.

@kirklund kirklund merged commit 636bea3 into apache:develop Oct 19, 2021
@kirklund kirklund deleted the GEODE-9713-recommit branch October 19, 2021 00:16
demery-pivotal pushed a commit to demery-pivotal/geode that referenced this pull request Oct 19, 2021
Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.
Bill pushed a commit to Bill/geode that referenced this pull request Nov 23, 2021
Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.

(cherry picked from commit 636bea3)
Bill pushed a commit to Bill/geode that referenced this pull request Nov 23, 2021
Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.

(cherry picked from commit 636bea3)
Bill pushed a commit to Bill/geode that referenced this pull request Nov 23, 2021
Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.

(cherry picked from commit 636bea3)
Bill pushed a commit that referenced this pull request Nov 25, 2021
Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.

(cherry picked from commit 636bea3)
Bill pushed a commit that referenced this pull request Nov 25, 2021
Restores thread count support to ExecutorServiceRule, and adds it to
DistributedExecutorServiceRule.

PROBLEM

ExecutorService rules currently create a newCachedThreadPool which
creates new threads as needed.

Some usages would benefit from the option of specifying a threadCount
limit which would create a newFixedThreadPool that reuses a fixed
number of threads.

SOLUTION

Add optional threadCount creation parameter to both ExecutorServiceRule
and DistributedExecutorServiceRule.

Creating a ExecutorService rule without a threadCount will still create a
newCachedThreadPool. Using a threadCount will now create a
newFixedThreadPool.

(cherry picked from commit 636bea3)
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.

3 participants