Skip to content

Minimize the threadpool for client commands #2193

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

Merged
merged 8 commits into from
Apr 8, 2022

Conversation

tobim
Copy link
Member

@tobim tobim commented Apr 6, 2022

Running many sub-commands in parallel can quickly result in very large numbers of threads being created. This can become a problem when these processes are in a restricted environment, and some start receiving "resource unavailable" errors. We can mitigate the problem by using only the minimum amount of workers for client commands, which don't need a these resources anyways.

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Running many sub-commands in parallel can quickly result in very
large numbers of threads being created. This can become a problem
when these processes are in a restricted environment, and some
start receiving "resource unavailable" errors. We can mitigate the
problem by using only the minimum amount of workers for client
commands, which don't need a these resources anyways.
@tobim tobim added maintenance Tasks for keeping up the infrastructure performance Improvements or regressions of performance labels Apr 6, 2022
@tobim tobim requested a review from a team April 6, 2022 06:51
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Some initial thoughts. Will do some tests locally with this as well to see if any client commands run into issues now. There's always at least two actors running if we have a remote connection, so I'd like to test whether the new default should be 2 rather than 1.

tobim and others added 3 commits April 6, 2022 07:58
@dominiklohmann dominiklohmann force-pushed the topic/client-command-threads branch from afa0d99 to cf96f46 Compare April 6, 2022 09:12
dominiklohmann and others added 3 commits April 6, 2022 11:16
Testing revealed that import commands would easily get stuck if
only one thread is availble.
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This actually improved performance for client commands for me a noticeable bit. Thanks!

@tobim tobim merged commit 1ca9d40 into master Apr 8, 2022
@tobim tobim deleted the topic/client-command-threads branch April 8, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Tasks for keeping up the infrastructure performance Improvements or regressions of performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants