Skip to content

Conversation

Hazhzeng
Copy link
Contributor

Description

This is a dependency of enabling async thread pool. Getting this change into the codebase will allow us configure the threadpool more easily when benchmarking the async threadpool performance.

Fixes #675


Now we allow customer to configure number of thread in the synchronous threadpool by setting PYTHON_THREADPOOL_THREAD_COUNT application setting. If the application settings is not set, we will default it to 1.

I'm wondering whether we should call this PYTHON_SYNC_THREADPOOL_THREAD_COUNT.
If we introduce async thread pool later on, should we introduce a new PYTHON_ASYNC_THREADPOOL_THREAD_COUNT app settings? It is possible that if a function app mixes both sync and async functions, the actual number of threads will be 2x. Adding this new setting will has a better the transparency for customer to control their function app, but will make the app setting more complicate.

In my opinion, both async threadpool and sync threadpool should use the value in PYTHON_THREADPOOL_THREAD_COUNT.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made and CI is passing.

Quality of Code and Contribution Guidelines


# Feature Flags (app settings)
PYTHON_ROLLBACK_CWD_PATH = "PYTHON_ROLLBACK_CWD_PATH"
PYTHON_THREADPOOL_THREAD_COUNT = "PYTHON_THREADPOOL_THREAD_COUNT"
Copy link
Member

Choose a reason for hiding this comment

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

@stefanushinardi - Could you please check if you want to change the name?

Choose a reason for hiding this comment

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

LGTM - putting my reasoning here for future reference: it makes sense to put the PYTHON_* prefix for this as without it users might get confused and use this config for other languages

@Hazhzeng Hazhzeng requested a review from vrdmr September 24, 2020 20:20
Copy link
Member

@vrdmr vrdmr left a comment

Choose a reason for hiding this comment

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

LGTM.

some nits.

@Hazhzeng Hazhzeng merged commit 825f0b0 into dev Sep 24, 2020
@Hazhzeng Hazhzeng deleted the hazeng/async-threadpool branch September 24, 2020 21:28
vrdmr added a commit that referenced this pull request Oct 2, 2020
* Add PYTHON_THREADPOOL_THREAD_COUNT app setting (#744)
* Adding support for debug logs in executed functions. (#745)
* Bumping up the version to 1.1.6 (#748)
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.

Investigate making the number of threads in the Azure Functions Python Worker thread pool configurable

3 participants