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.

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

Concurrent Test Execution and maximum thread count #169

Closed
carlospzurita opened this issue Aug 30, 2018 · 4 comments · Fixed by #193
Closed

Concurrent Test Execution and maximum thread count #169

carlospzurita opened this issue Aug 30, 2018 · 4 comments · Fixed by #193
Assignees
Labels
Type: create EIP Create an EIP for this Enhancement

Comments

@carlospzurita
Copy link
Contributor

During some stress testing of the validator, related to etf-validator/governance#14, we came upon some issues with concurrent requests. We observed that at a certain number of requests pooled, the system crashes and the test run is stoppped.

Using JMeter, we start a test run with seven tests (test suite Conformance Class 1-7), launching the same request iteratively during 2 minutes.

The exception as it appears in the server log is:

Task java.util.concurrent.FutureTask@4170493d
 rejected from java.util.concurrent.ThreadPoolExecutor@669f091a[Running, pool size = 12, active threads = 12, queued tasks = 12, completed tasks = 111]
    at de.interactive_instruments.etf.webapp.controller.TestRunController.initAndSubmit(TestRunController.java:262)
    at de.interactive_instruments.etf.webapp.controller.TestRunController.start(TestRunController.java:543)

Looking for the source of this issue, we found in the class etf-webapp/src/main/java/de/interactive_instruments/etf/webapp/controller/TestRunController.java, line 108, that the maximum number of threads available is set using the number of cores from the processor

etf-webapp/src/main/java/de/interactive_instruments/etf/webapp/controller/TestRunController.java

Changing manually this value allowed us to pool more requests, up to 100, without any performance issue. ¿Maybe this number could be set in a properties file, or make it dynamically set? This can open up the possibility of elastic resource allocation.

@jonherrmann jonherrmann self-assigned this Aug 30, 2018
@jonherrmann jonherrmann added the Type: create EIP Create an EIP for this Enhancement label Aug 30, 2018
jonherrmann added a commit to etf-validator/legacy-etf-core that referenced this issue Sep 1, 2018
Add option to set the queue size of the TaskPoolRegistry

etf-validator/etf-webapp#169
@jonherrmann
Copy link
Collaborator

I think it's a good idea to introduce a parameter to control this behaviour.

Since threads in the queue can also block resources, we should also introduce one parameter to explicitly set the maximum size of the queue:

etf.testruns.max.threads
etf.testruns.max.queued

The default for etf.testruns.max.threadsshould be set to to the CPU cores. I would propose a default queue size of three times the CPU cores.

However there is a design flaw in the core: the TestTaskRegistry ctor does not allow to pass a parameter for the queue size. It's always identical to the number of max threads. So in your load test scenario you have increased the parallelism in order to increase the queue size. This might work for small datasets but might lead to issues with bigger ones.

In the next etf-core version 1.1.0 you can call this ctor and set the queue size. Change this line to use the updated library.

@carlospzurita
Copy link
Contributor Author

Thanks for your comments @jonherrmann, we will try that constructor and keep testing. I suppose that the code of this new version of the etf-core is the one in the branch next, right?

@jonherrmann
Copy link
Collaborator

Correct, Carlos. You can either build + install the etf-core locally and use it or just use the library from the ii repository (the snapshot repositories always contain the latest libraries from the next branches) by changing the line to:

    compile group: 'de.interactive_instruments.etf', name: 'etf-core', version: '1.1.0' + project.snapshotSuffix

in the etf-webapp (this has not been changed in the etf-webapp next branch, yet). These are the lines, that have been changed in the etf-core.

@jonherrmann
Copy link
Collaborator

Closing this in favour of EIP 48

@jonherrmann jonherrmann linked a pull request Aug 1, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: create EIP Create an EIP for this Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants