ARTEMIS-4522 Dedicated thread pool for flow-control-executor

[MrEasy]

Adds a dedicated thread pool for flow-control-executor to resolve slow-consumer-handling not being able to run in good time if common thread-pool is exhausted

[MrEasy]

Any help/comment rg. this would be good - or deemed unneccessary?

[clebertsuconic]

If you set a smaller thread pool you could get other issues...

you fixed one case by adding a new thread pool for flow control.

When you hit another feature not working because the thread pool is saturated, are you going to add another pool to that feature also?

Having a warn that the pool is saturated and suggestion to increase would probably be a better feature IMO.

[MrEasy]

Hi,

When you hit another feature not working because the thread pool is saturated, are you going to add another pool to that feature also?

If you reach the configured limit, I think everyone is clear that you will get delays until resources are freed. Our goal here was to address the issue that was more unexpected to us, which lead to the big 10 seconds delays based on the flow-control explicitly.
You could argue, that if going into slow-consumer-handling is not aiming for highest throughput anyhow, but having consumer-window-size>0 this has another disadvantage for us (see lengthy explanatin by Sascha in slack).
So goal for us here was to stick to the slow-consumer-handling, but avoiding the big 10 seconds influence of the full pool on it.

Having a warn that the pool is saturated and suggestion to increase would probably be a better feature IMO.

I think that would also be an excellent improvement, since now it is not easily visible when the client thread-pool reaches its limit.

[jbertram]

I've talked with @MrEasy (and one of his colleague's) in Slack about his use-case. This solution seems reasonable to me. If you want to constrain the "normal" thread pool but still ensure that flow-control works without extreme latency then we need something like this.

In the future we could probably ditch these kinds of things using Virtual Threads.

[jbertram]

@MrEasy, you've got a handful of checkstyle violations:

[INFO] There are 8 errors reported by Checkstyle 10.11.0 with /home/runner/work/activemq-artemis/activemq-artemis/artemis-core-client/../etc/checkstyle.xml ruleset.
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryImpl.java:[196,4] (blocks) LeftCurly: '{' at column 4 should be on the previous line.
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[194,4] (blocks) LeftCurly: '{' at column 4 should be on the previous line.
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[197,7] (blocks) RightCurly: '}' at column 7 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally).
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[204,7] (blocks) RightCurly: '}' at column 7 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally).
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[209,10] (blocks) LeftCurly: '{' at column 10 should be on the previous line.
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[212,13] (blocks) LeftCurly: '{' at column 13 should be on the previous line.
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[220,10] (blocks) RightCurly: '}' at column 10 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally).
Error:  src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java:[228,13] (blocks) LeftCurly: '{' at column 13 should be on the previous line.

You can check this yourself locally if you want via:

mvn -Pdev install

See the hacking guide for more details.

Also, your commits should be squashed.

[MrEasy]

MrEasy, you've got a handful of checkstyle violations:
...
Also, your commits should be squashed.

Reduced violence and squashed

[jbertram]

@MrEasy, I followed-up on this with #4708. The new PR has an additional commit which drastically simplifies the test and finishes the implementation with configuration properties, etc.