Don't execute queries in background when max_threads is 0#41586
Merged
eileencodes merged 1 commit intorails:mainfrom Mar 2, 2021
Merged
Conversation
kamipo
reviewed
Mar 2, 2021
Member
There was a problem hiding this comment.
How about adding # :nodoc:?
In other words, is it intended to be exposed in the API doc?
In the multi_thread_pool executor it's possible to have a database configuration that wouldn't work well with a thread exectutor. This change checks the `@db_config.max_threads` and if it is not greater than 0 we won't create a thread pool for that configuration. Database configurations without a proper value set for `max_threads` will return a `nil` async executor and will run queries for those connection pools in the foreground. This allows applications to more easily control which database configurations in their app support async queries. In this setup one database coule be configured to run parallel queries while all other databases run queries in the foreground.
b6e2775 to
6b9027b
Compare
abhaynikam
added a commit
to abhaynikam/rails
that referenced
this pull request
Mar 3, 2021
PR: rails#41586 While reading the change noticed the test cases used assert for comparsion. The problem here is assert would never raise error if the expected and actual value do not match. After changing it to the assert_equal, async_query_executor test cases started failing because the connection was already established before the spec was ran and setting global `global_executor_concurrency` did not reset the global_thread_pool_async_query_executor Changes to assert_equal also broke the async_query_executor = :multi_thread_pool specs as the max_length defaults to pool(i.e: 5) in the database configuration. PR attemps to fix all the above mentioned issues in the asynchronous_queries_test.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the multi_thread_pool executor it's possible to have a database
configuration that wouldn't work well with a thread exectutor. This
change checks the
@db_config.max_threadsand if it is not greater than0 we won't create a thread pool for that configuration. Database
configurations without a proper value set for
max_threadswill returna
nilasync executor and will run queries for those connection poolsin the foreground. This allows applications to more easily control which
database configurations in their app support async queries. In this
setup one database coule be configured to run parallel queries while all
other databases run queries in the foreground.
cc/ @jhawthorn @rafaelfranca @byroot