Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Service config in maps #3624

Merged
merged 14 commits into from
Apr 6, 2022
Merged

Service config in maps #3624

merged 14 commits into from
Apr 6, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Apr 4, 2022

Store service options in maps with defaults. Other changes for each service are listed below.

service_admin_extra:

  • removed redundant defaults from TOML

service_domain_db:

  • added missing config tests
  • refactored big tests
  • some helpers need the DB pool to be explicitly provided now, as there are no config values when the service is not loaded

service_mongoose_system_metrics:

  • removed the shorter intervals for CI as they were not used anyway (the explicit values in TOML were overriding them). We can set the shorter values for CI explicitly if we want to, but I see no motivation to change it now.
  • refactored big tests
  • removed redundant defaults from TOML

Other changes:

  • Added utilities for managing services dynamically: dynamic_services, which uses the new mongoose_service:replace_services/2. This is very similar to dynamic_modules, which uses mongoose_modules:replace_modules/2. There are differences between services and modules that make direct code reuse more difficult, so the code is just made to be consistent.

@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #3624 (cc1c119) into master (6fcb515) will decrease coverage by 0.00%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #3624      +/-   ##
==========================================
- Coverage   81.01%   81.00%   -0.01%     
==========================================
  Files         427      427              
  Lines       32135    32126       -9     
==========================================
- Hits        26033    26025       -8     
+ Misses       6102     6101       -1     
Impacted Files Coverage Δ
src/admin_extra/service_admin_extra.erl 100.00% <ø> (ø)
src/domain/mongoose_domain_db_cleaner.erl 66.66% <ø> (-3.93%) ⬇️
src/mongoose_cluster.erl 75.28% <ø> (ø)
src/domain/mongoose_domain_sql.erl 92.03% <50.00%> (-0.28%) ⬇️
...system_metrics/service_mongoose_system_metrics.erl 78.43% <94.11%> (-5.44%) ⬇️
src/domain/mongoose_domain_api.erl 100.00% <100.00%> (ø)
src/domain/mongoose_domain_core.erl 89.13% <100.00%> (+0.49%) ⬆️
src/domain/service_domain_db.erl 83.33% <100.00%> (ø)
src/mongoose_service.erl 96.92% <100.00%> (+0.55%) ⬆️
src/pubsub/mod_pubsub_db.erl 57.14% <0.00%> (-28.58%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fcb515...cc1c119. Read the comment docs.

Also: simplify defaults
- The CI-specific report intervals had no effect because the default
  ones were set explicitly in the TOML file anyway
@mongoose-im

This comment was marked as outdated.

Follow the logic from mongoose_modules.
The tests are very similar to the ones for replace_modules,
but it seems that there are enough differences to keep them separate.
Follow the logic from dynamic_modules.
Code resue does not seem to make much sense because pof the differences.
- Use maps with defaults
- Do not skip tests if system_metrics are not running
- Use dynamic_services
@chrzaszcz chrzaszcz force-pushed the service-config-in-maps branch from 7799927 to 82658d2 Compare April 5, 2022 10:19
@mongoose-im

This comment was marked as outdated.

- Provide the db pool explicitly to test defaults that are used when
  the service is not loaded, and thus the config is not available.
It was previously untested.
It is added to the 'host_types' file test case as well.
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the service-config-in-maps branch from d63948c to 0ae1905 Compare April 5, 2022 16:13
@mongoose-im

This comment was marked as outdated.

This way it is easier to restart it in tests.
- Use maps with defaults
- Use dynamic_services
- Provide the DB pool explicitly when the service is not loaded
@chrzaszcz chrzaszcz force-pushed the service-config-in-maps branch from 0ae1905 to cc1c119 Compare April 5, 2022 17:20
@mongoose-im
Copy link
Collaborator

mongoose-im commented Apr 5, 2022

riak_mnesia_24 / riak_mnesia / cc1c119
Reports root


small_tests_24 / small_tests / cc1c119
Reports root / small


small_tests_23 / small_tests / cc1c119
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / cc1c119
Reports root/ big
OK: 2857 / Failed: 1 / User-skipped: 133 / Auto-skipped: 0

bosh_SUITE:essential:accept_higher_hold_value
{error,
  {{assertEqual,
     [{module,bosh_SUITE},
      {line,251},
      {expression,"get_bosh_sessions ( )"},
      {expected,[]},
      {value,
        [{bosh_session,<<"4c361b8af5df4d5949dc7affef5dadd940ba1cdb">>,
           <8633.6848.0>}]}]},
   [{bosh_SUITE,accept_higher_hold_value,1,
      [{file,"/home/circleci/project/big_tests/tests/bosh_SUITE.erl"},
       {line,251}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1292}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1224}]}]}}

Report log


dynamic_domains_mysql_redis_24 / mysql_redis / cc1c119
Reports root/ big
OK: 2841 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / cc1c119
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / cc1c119
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / cc1c119
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / cc1c119
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / cc1c119
Reports root/ big
OK: 1547 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / cc1c119
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / cc1c119
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / cc1c119
Reports root/ big
OK: 3227 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / cc1c119
Reports root/ big
OK: 1854 / Failed: 0 / User-skipped: 368 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / cc1c119
Reports root/ big
OK: 3232 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / cc1c119
Reports root/ big
OK: 2858 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / cc1c119
Reports root/ big
OK: 1697 / Failed: 0 / User-skipped: 367 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review April 6, 2022 07:01
Copy link
Contributor

@Premwoik Premwoik left a comment

Choose a reason for hiding this comment

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

All looks great :) I left two minor suggestions.

@Premwoik Premwoik merged commit 40d41ee into master Apr 6, 2022
@Premwoik Premwoik deleted the service-config-in-maps branch April 6, 2022 09:35
@Premwoik Premwoik added this to the 5.1.0 milestone May 25, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants