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

MONGOID-5445 Add load_async to criteria #5454

Merged
merged 37 commits into from
Sep 30, 2022

Conversation

comandeo
Copy link
Contributor

@comandeo comandeo commented Sep 7, 2022

https://jira.mongodb.org/browse/MONGOID-5445

This PR implements Mongoid version of ActiveRecord load_async.

This PR also introduces async_query_executor and global_executor_concurrency configuration options.

Unlike Rails version, this PR does not support :multi_thread_pool option for the async_query_executor. As per Rails docs ":multi_thread_pool will use one pool per database, and each pool size can be configured individually in database.yml through the max_threads and min_thread properties." I am not sure if we need this functionality in Mongoid, but we can implement it if required.

@comandeo comandeo requested review from p-mongo and Neilshweky and removed request for p-mongo September 14, 2022 10:34
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
lib/mongoid/contextual/mongo/documents_loader.rb Outdated Show resolved Hide resolved
spec/mongoid/config_spec.rb Show resolved Hide resolved

describe '#execute' do
it 'does not change state' do
prev_started = subject.started?
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you assert what the state is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initial state is tested above, in #initialize specs. I am not sure if we should re-test it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would assert initial states.

Copy link
Contributor Author

@comandeo comandeo Sep 14, 2022

Choose a reason for hiding this comment

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

If we add an assertion for the initial state here, and then for some reason setting initial state in the constructor is broken, we will see that the test Mongoid::Contextual::Mongo::DocumentsLoader#execute does not change state fails, even though the #execute method is fine.
The initial state is set by the #initialize method, which is tested above.

spec/mongoid/contextual/mongo/documents_loader_spec.rb Outdated Show resolved Hide resolved
@comandeo comandeo marked this pull request as ready for review September 14, 2022 14:47
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
lib/mongoid/config.rb Outdated Show resolved Hide resolved
lib/mongoid/config.rb Show resolved Hide resolved
lib/mongoid/contextual/mongo/documents_loader.rb Outdated Show resolved Hide resolved
lib/mongoid/contextual/mongo/documents_loader.rb Outdated Show resolved Hide resolved
lib/mongoid/contextual/mongo/documents_loader.rb Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
docs/reference/queries.txt Outdated Show resolved Hide resolved
lib/mongoid/config/validators/async_query_executor.rb Outdated Show resolved Hide resolved
# to be used to execute document loading tasks.
def self.global_thread_pool_async_query_executor
concurrency = Mongoid.global_executor_concurrency || 4
@@global_thread_pool_async_query_executor ||= Concurrent::ThreadPoolExecutor.new(
Copy link
Contributor

Choose a reason for hiding this comment

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

The caching here makes it so that if the concurrency is altered at runtime after any query is executed with load_async, the concurrency change will have no effect and the original value will continue to be used. This should be mentioned in the docs or this method should check if the concurrency changed and recreated the thread pool executor in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we generally consider changing Mongoid configuration options at runtime as a valid usage?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am unaware of any existing prohibitions on altering Mongoid configuration options at any time. While this is not something applications would typically do, I would say it is permitted.

Furthermore, "at runtime" includes the time when initializers run. An application can issue a query from one initializer and alter config option in another initializer which happens to run later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

comandeo-mongo and others added 10 commits September 27, 2022 18:27
…M association (mongodb#5467)

* MONGOID-5164 see if test fails when executing a query

* MONGOID-5164 fix tests

* omit apply_scope
* RUBY-2896 use BSON _bson_to_i method if it exists

* RUBY-2846 change bson_master gemfile to test changes

* Update gemfiles/bson_master.gemfile
… in default scope (mongodb#5474)

* MONGOID-5227 Add test and adjust docs for dotted attribute assignment in default scope

* MONGOID-5227 adjust docs

* MONGOID-5227 clarify the docs and add tests

* Update docs/reference/queries.txt

Co-authored-by: Oleg Pudeyev <[email protected]>

Co-authored-by: Oleg Pudeyev <[email protected]>
@p-mongo
Copy link
Contributor

p-mongo commented Sep 28, 2022

Thank you, I think the previous pool needs to have .shutdown called on it as per the termination section of https://ruby-concurrency.github.io/concurrent-ruby/master/Concurrent/ThreadPoolExecutor.html. I am guessing waiting for the pool to terminate is not necessary.

@comandeo comandeo merged commit dc0d505 into mongodb:master Sep 30, 2022
@comandeo comandeo deleted the 5445-load-async branch September 30, 2022 11:42
comandeo pushed a commit that referenced this pull request Sep 30, 2022
Co-authored-by: Neil Shweky <[email protected]>
Co-authored-by: Oleg Pudeyev <[email protected]>
jamis added a commit that referenced this pull request Mar 14, 2023
…) to master (#5561)

* MONGOID-5334 ensure localized demongoization works with symbol keys (#5416)

* MONGOID-5334 ensure localized demongoization works with symbol keys

* MONGOID-5334 add type check

* MONGOID-5334 change back type check

* MONGOID-5437 extract fallback enabling logic into macro (#5428)

* MONGOID-5438 initial changes from Johnny's PR

* MONGOID-5438 additional changes from johnny's
PR

* MONGOID-5437 fix tests

* MONGOID-5437 fix uniqueness test

* MONGOID-5334 enforce Hash type in localized demongoize (#5430)

* MONGOID-5334 add type check

* MONGOID-5334 change back type check

* MONGOID-5334 enforce Hash type in localized demongoize

* MONGOID-5456 cast castable values for integer/float/big decimal (#5431)

* MONGOID-5456 cast castable values for integer/float/big decimal

* MONGOID-5456 assert duration

* MONGOID-4403 Support Dirty Tracking changed from/to (#5432)

* MONGOID-4403 Support Dirty Tracking changed from/to

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-4403 add false example

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5456 Add documentation for responding to to_* (#5434)

* MONGOID-5456 Add documentation for responding to to_*

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5461 Change custom field options example (#5438)

* MONGOID-5461 Change custom field options example

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5458 Document fallbacks option on localized fields (#5439)

* MONGOID-5460 Update custom field type protocol documentation (#5441)

* MONGOID-5457 Document + example of non-String field types in localized fields (#5444)

* MONGOID-4698 update warning when overriding criteria methods in scope (#5442)

* MONGOID-4698 update warning when overriding criteria methods in scope

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5417 Fix memory leak when call 'with' (#5409)

* MONGOID-4528 Add more dirty methods (#5440)

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-4698 Correct warning to point out that klass.band doesnt conflict (#5450)

* MONGOID-5226 Allow setting "store_in collection" on document subclass (#5449)

* MONGOID-5226 Allow setting "store_in collection" on document subclass

* remove comment

* MONGOID-5226 add docs and testing

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5226 answer comments

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5293 put legacy_attributes in 7.5 defaults (#5425)

* MONGOID-5293 put legacy_attributes in 7.5

* switch order

* MONGOID-5293 fix defaults tests

* MONGOID-5438 Implement local override for i18n parameters (#5427)

* MONGOID-5438 Implement local override for i18n parameters

* MONGOID-5438 get rid of enfore_available_locale changes

* MONGOID-5438 get rid of before alls

* MONGOID-5438 fix tests caused by rspec precedence

* MONGOID-5438 attempt to fix tests when fallbacks disabled

* MONGOID-5438 make fallbacks default to return [self]

* MONGOID-5438 reset fallbacks

* MONGOID-5438 attempt to fix tests

* MONGOID-5438 retry_test

* MONGOID-5438 undefine fallback methods after fallback tests

* MONGOID-5438 add back defaults

* MONGOID-5438 use new macros

* MONGOID-5438 use before and after suite

* MONGOID-5438 update mrss

* MONGOID-5438 remove suite checks

* MONGOID-5370 Add collection options support (#5452)

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5474 add readonly! and raise on save/update (#5455)

* MONGOID-5474 add readonly! and raise on save/update

* MONGOID-5474 flip flag for 9.0 and add default

* MONGOID-5474 fix error tests

* Update spec/mongoid/config_spec.rb

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

* Update lib/mongoid/config.rb

Co-authored-by: Oleg Pudeyev <[email protected]>

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5474/MONGOID-5478 add docs and release notes (#5457)

* MONGOID-5474 add docs and release notes

* MONGOID-5474 use read-only

* MONGOID-5474 add configuration docs

* MOGOID-5474 switch order of flag docs

* add MONGOID-5477 use case

* MONGOID-5477 / MONGOID-5474 / MONGOID-5478 add user test (#5459)

* MONGOID-5474 add user test

* MONGOID-5474 change to read-only

* MOGOID-5474 switch order of flag docs

* MONGOID-5476 update 8.1 installation version (#5464)

* MONGOID-5474 flip the feature flag in 8.1 (#5456)

* MONGOID-5474 flip the feature flag in 8.1

* MONGOID-5474 flip flag in configuration

* MONGOID-5474 update config test

* MONGOID-5227 Add test and adjust docs for dotted attribute assignment in default scope (#5474)

* MONGOID-5227 Add test and adjust docs for dotted attribute assignment in default scope

* MONGOID-5227 adjust docs

* MONGOID-5227 clarify the docs and add tests

* Update docs/reference/queries.txt

Co-authored-by: Oleg Pudeyev <[email protected]>

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5493: Update Gem::Specification team info (#5476)

* MONGOID-5312 Document attributes method in reference (#5477)

* MONGOID-5312 Document attributes method in reference

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5312 update languagw

Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5082 Create contributing guidelines (#5472)

* MONGOID-5082 Create contributing guidelines

* MONGOID-5082 propose test coverage

* MONGOID-5082 whitespace

* MONGOID-5082 update language

* add intro

* MONGOID-5445 Add load_async to criteria (#5454)

Co-authored-by: Neil Shweky <[email protected]>
Co-authored-by: Oleg Pudeyev <[email protected]>

* MONGOID-5291 Clarify whether :touch option is applicable for all updates (#5481)

* MONGOID-5261 Use config_override when a test requires a certain configuration option value (#5479)

* MONGOID-5261 Use config_override when a test requires a certain configuration option value

* fix syntax error

* MONGOID-5261 a little more cleanup

* MONGOID-5261 more cleanup

* more clean up

* MONGOID-5261 add persistence context override

* MONGOID-5261 add comment

* MONGOID-5100 allow selector arguments for .exists? (#5466)

* MONGOID-5100 allow selector arguments for .exists?

* MONGOID-5100 add examples to the docstrings

* MONGOID-5100 add docs and release notes

* Empty-Commit

* MONGOID-5100 allow false to be passed

* MONGOID-5100 update docs and docstrings

* MONGOID-5100 remove false as a param

* MONGOID-5100 put back nil/false

* MONGOID-5100 fix memory tests and fix limit == 0

* MONGOID-5262 Implement local time zone override for tests (#5480)

* MONGOID-5506 Update 8.x docs to document unscoped behavior changing in 9.0 (#5483)

* MONGOID-5506 Update 8.x docs to document unscoped behavior changing in 9.0

* Update lib/mongoid/scopable.rb

* RUBY-2942: Update documentation links (#5482)

* MONGOID-5363 Change #upsert to perform updating upsert rather than replacing upsert (#5492)

* MONGOID-5363 add :replace option to upsert method

* MONGOID-5363 add docs, release notes for 8.1/9.0

* MONGOID-5363 switch default in 8.1 (#5493)

* MONGOID-5363 update docstrings, flag default, and tests for 8.1

* specify default in mongoid 9

* MONGOID-5331 Document hash assignment to associations (#5494)

* MONGOID-5331 change buildable docs

* MONGOID-5331 add proxy tests and fix embedded_in proxy

* MONGOID-5331 update models

* Update spec/mongoid/association/embedded/embedded_in/proxy_spec.rb

* MONGOID-5331 update doc strings in batchable

* MONGOID-5331 add docs

* MONGOID-5331 add docs/release notes

* Apply suggestions from code review

* Update spec/mongoid/association/embedded/embedded_in/proxy_spec.rb

* RUBY-3097 add/use monotonic time for deadlines (#5491)

* RUBY-3097 add/use monotonic time for deadlines

* remove license and fix tests

* MONGOID-3834 Document association availability in custom getters and setters (#5503)

* MONGOID-3834 Document association availability in custom getters and setters

* add callbacks note

* MONGOID-5496: Update Getting Started (Rails) tutorial for Rails 7 (#5509)

* Clone Rails Tutorial and create variation for Rails 7

* Update Existing Application docs for Rails 7

* MONGOID-5532: Fix path to comments partial (#5511)

* MONGOID-5533: Unnecessary edits to Comment model in Rails Getting Started (#5512)

* Add note about workaround for MONGOID-4885 in Rails 6 docs

* Fix whitespace

* MONGOID-5521 migrate Mongoid docs to snooty (#5513)

* fix code blocks

* MONGOID-5521 fix all parse errors

* build docs with github action

* Revert "build docs with github action"

This reverts commit 9ffda44.

* MONGOID-5496: Update Rails 7 Tutorial (#5515)

* Remove `--skip-bundle` from _Optionally Skip Tests_ section

* MONGOID-5518: Add Puma dependency to Sinatra tutorial (#5516)

* MONGOID-5453 Add .none_of query method (#5524)

* "none_of" query method

* update docs and release notes

* fix underline for header

* MONGOID-5539: Fix example in aggregation reference documentation (#5536)

* Update aggregation.txt

* Update aggregation.txt

* MONGOID-5228 disallow _id to be updated on persisted documents (ported to 8.1-stable) (#5545)

* port #5542 to 8.1-stable

* test tweak

* update 8.1 release notes to mention immutable_ids

* MONGOID-5490: Deprecate :use_activesupport_time_zone (#5488)

* Deprecate the use_activesupport_time_zone_deprecated config option.

* Typo

* Update config.rb

* Update mongoid-8.1.txt

---------

Co-authored-by: shields <[email protected]>

* MONGOID-5509: Deprecate all feature flags which were introduced in Mongoid 7.x (#5489)

* Deprecate the use_activesupport_time_zone_deprecated config option.

* Typo

* Mongoid 8.1 should deprecate all feature flags which were introduced in the 7.x series, with intent to remove them in Mongoid 9.0.

---------

Co-authored-by: shields <[email protected]>

* Remove :use_activesupport_time_zone option + improve readme around timezone usage given the change

* Remove deprecated :broken_aggregables config option

* Remove deprecated :broken_alias_handling config option

* Remove deprecated :broken_and config option

* Remove deprecated :broken_scoping config option

* Remove deprecated :broken_updates config option

* Remove deprecated :compare_time_by_ms config option

* Remove deprecated :legacy_attributes config option

* Remove deprecated :legacy_pluck_distinct config option

* Remove deprecated :legacy_triple_equals config option

* Remove deprecated :object_id_as_json_oid config option

* Remove deprecated :overwrite_chained_operators config option

* Drop support for config.load_defaults versions "7.3", "7.4", "7.5", as their options are no longer available.

* Fix broken spec in Mongoid 8.1

---------

Co-authored-by: Neil Shweky <[email protected]>
Co-authored-by: Oleg Pudeyev <[email protected]>
Co-authored-by: Dmitry Rybakov <[email protected]>
Co-authored-by: Alex Bevilacqua <[email protected]>
Co-authored-by: Jamis Buck <[email protected]>
Co-authored-by: Jamis Buck <[email protected]>
Co-authored-by: shields <[email protected]>
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.

4 participants