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

Initialise domain workers in the supervision tree instead of manually #4069

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented Jul 27, 2023

Follow cleaner OTP supervision conventions, we want these workers supervised by their common supervisor, started cleanly within the supervision tree.

@NelsonVides NelsonVides force-pushed the domain_workers_supervisor branch 2 times, most recently from 3b5ca2f to fd48ea6 Compare July 27, 2023 13:31
@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (6cef853) 83.88% compared to head (73ef2c5) 83.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4069      +/-   ##
==========================================
+ Coverage   83.88%   83.90%   +0.01%     
==========================================
  Files         526      527       +1     
  Lines       33181    33158      -23     
==========================================
- Hits        27834    27820      -14     
+ Misses       5347     5338       -9     
Files Changed Coverage Δ
src/domain/mongoose_domain_api.erl 97.53% <ø> (-0.20%) ⬇️
src/domain/mongoose_domain_core.erl 87.34% <ø> (-1.79%) ⬇️
src/domain/mongoose_lazy_routing.erl 86.17% <ø> (+1.33%) ⬆️
src/domain/mongoose_subdomain_core.erl 91.83% <ø> (+2.11%) ⬆️
src/ejabberd_app.erl 93.75% <ø> (-0.08%) ⬇️
src/domain/mongoose_domain_sup.erl 100.00% <100.00%> (ø)
src/ejabberd_sup.erl 86.20% <100.00%> (+0.49%) ⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mongoose-im

This comment was marked as outdated.

@NelsonVides NelsonVides changed the title Prepare supervisor for domain workers Initialise domain workers in the supervision tree instead of manually Jul 27, 2023
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@NelsonVides NelsonVides marked this pull request as ready for review July 27, 2023 16:44
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Jul 28, 2023

small_tests_24 / small_tests / a2f641a
Reports root / small


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / a2f641a
Reports root/ big
OK: 369 / Failed: 0 / User-skipped: 38 / Auto-skipped: 0


small_tests_25 / small_tests / a2f641a
Reports root / small


small_tests_25_arm64 / small_tests / a2f641a
Reports root / small


ldap_mnesia_24 / ldap_mnesia / a2f641a
Reports root/ big
OK: 2258 / Failed: 0 / User-skipped: 827 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a2f641a
Reports root/ big
OK: 4221 / Failed: 0 / User-skipped: 82 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a2f641a
Reports root/ big
OK: 4221 / Failed: 0 / User-skipped: 82 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / a2f641a
Reports root/ big
OK: 2258 / Failed: 0 / User-skipped: 827 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / a2f641a
Reports root/ big
OK: 4195 / Failed: 0 / User-skipped: 108 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a2f641a
Reports root/ big
OK: 4218 / Failed: 0 / User-skipped: 85 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / a2f641a
Reports root/ big
OK: 4603 / Failed: 1 / User-skipped: 89 / Auto-skipped: 0

mod_global_distrib_SUITE:hosts_refresher:test_host_refreshing
{error,
  {{trees_for_connections_present,true,[{times,100,false}],ok},
   [{mongoose_helper,do_wait_until,2,
      [{file,"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
       {line,357}]},
    {mod_global_distrib_SUITE,test_host_refreshing,1,
      [{file,
         "/home/circleci/project/big_tests/tests/mod_global_distrib_SUITE.erl"},
       {line,384}]},
    {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


internal_mnesia_25 / internal_mnesia / a2f641a
Reports root/ big
OK: 2404 / Failed: 0 / User-skipped: 681 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / a2f641a
Reports root/ big
OK: 4604 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / a2f641a
Reports root/ big
OK: 4588 / Failed: 1 / User-skipped: 103 / Auto-skipped: 1

metrics_c2s_SUITE:single:presence_direct_one
{error,
  {{xmppPresenceReceived,
     {value,2842},
     [{times,25,
        {error,
          {badmatch,{value,2843}},
          [{metrics_helper,assert_counter,3,
             [{file,
              "/home/circleci/project/big_tests/tests/metrics_helper.erl"},
            {line,36}]},
           {mongoose_helper,do_wait_until,2,
             [{file,
              "/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
            {line,360}]},
           {metrics_c2s_SUITE,'-presence_direct_one/1-fun-0-',2,
             [{file,
              "/home/circleci/project/big_tests/tests/metrics_c2s_SUITE.erl"},
            {line,143}]},
           {escalus_story,story,4,
             [{file,
              "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
            {line,72}]},
           {test_server,ts_tc,3,
             [{file,"test_server.erl"},{line,1782}]},
           {test_server,run_test_case_eval1,6,
             [{file,"test_server.erl"},{line,1291}]},
           {test_server,run_test_case_eval,9,
             [{file,"test_server.erl"},{line,1223}]}]}}],
     ok},
   [{mongoose_helper,do_wait_until,2,
      [{file,"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
       {line,357}]},
    {metrics_c2s_SUITE,'-presence_direct_one/1-fun-0-',2,
      [{file,
         "/home/circleci/project/big_tests/tests/metrics_c2s_SUITE.erl"},
       {line,143}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/...

Report log


mssql_mnesia_25 / odbc_mssql_mnesia / a2f641a
Reports root/ big
OK: 4601 / Failed: 0 / User-skipped: 92 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / a2f641a
Reports root/ big
OK: 4590 / Failed: 0 / User-skipped: 103 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / a2f641a
Reports root/ big
OK: 4604 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

mongoose_domain_core:start(),
mongoose_subdomain_core:start(),
mongoose_lazy_routing:start().
mongoose_domain_core:start_link(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linking is something we normally want to avoid for the testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this change

@@ -46,7 +46,11 @@
start() ->
Pairs = get_static_pairs(),
AllowedHostTypes = mongoose_config:get_opt(host_types),
start(Pairs, AllowedHostTypes).
ChildSpec =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is testing version of start/2 used anywhere?
Please do not duplicate ChildSpec definition, start/0 should call start/2 with default parameters.

@@ -43,7 +43,7 @@
terminate/2,
code_change/3]).

-ignore_xref([start_link/0, stop/0, sync/0]).
-ignore_xref([start_link/0, start/0, stop/0, sync/0]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to add start/0 now? What has changed?

@@ -36,7 +36,13 @@ init_per_suite(Config) ->
ok = mnesia:create_schema([node()]),
ok = mnesia:start(),
[mongoose_config:set_opt(Key, Value) || {Key, Value} <- opts()],
mongoose_domain_api:init(),
Self = self(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

just eliminate linking for unit testing.

Follow cleaner OTP supervision conventions,
we want these workers supervised.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Aug 2, 2023

small_tests_24 / small_tests / 73ef2c5
Reports root / small


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 73ef2c5
Reports root/ big
OK: 369 / Failed: 0 / User-skipped: 38 / Auto-skipped: 0


small_tests_25 / small_tests / 73ef2c5
Reports root / small


small_tests_25_arm64 / small_tests / 73ef2c5
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 73ef2c5
Reports root/ big
OK: 2258 / Failed: 0 / User-skipped: 827 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 73ef2c5
Reports root/ big
OK: 4221 / Failed: 0 / User-skipped: 82 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 73ef2c5
Reports root/ big
OK: 2258 / Failed: 0 / User-skipped: 827 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 73ef2c5
Reports root/ big
OK: 4221 / Failed: 0 / User-skipped: 82 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 73ef2c5
Reports root/ big
OK: 4195 / Failed: 0 / User-skipped: 108 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 73ef2c5
Reports root/ big
OK: 2403 / Failed: 1 / User-skipped: 681 / Auto-skipped: 0

pubsub_SUITE:tree+basic:publish_only_retract_items_scope_test
{error,{{badmatch,false},
    [{pubsub_tools,check_response,2,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,444}]},
     {pubsub_tools,receive_response,3,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,434}]},
     {pubsub_tools,receive_and_check_response,4,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,424}]},
     {pubsub_SUITE,'-publish_only_retract_items_scope_test/1-fun-0-',2,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_SUITE.erl"},
            {line,645}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1291}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1223}]}]}}

Report log


pgsql_mnesia_24 / pgsql_mnesia / 73ef2c5
Reports root/ big
OK: 4604 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 73ef2c5
Reports root/ big
OK: 4218 / Failed: 0 / User-skipped: 85 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 73ef2c5
Reports root/ big
OK: 4604 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 73ef2c5
Reports root/ big
OK: 4590 / Failed: 0 / User-skipped: 103 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 73ef2c5
Reports root/ big
OK: 4601 / Failed: 0 / User-skipped: 92 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 73ef2c5
Reports root/ big
OK: 2404 / Failed: 0 / User-skipped: 681 / Auto-skipped: 0

mongoose_config:unset_opt({auth, host_type()}),
mnesia:stop(),
mnesia:delete_schema([node()]).

init_per_testcase(_TC, Config) ->
mongoose_domain_core:start_link([{domain(), host_type()}], []),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment that mongoose_domain_core "stops" because every TC runs in a separate process and linking ensure termination of mongoose_domain_core, and parallel execution of TCs is not possible.

[mongoose_config:unset_opt(Key) || {Key, _Value} <- opts()],
mnesia:stop(),
mnesia:delete_schema([node()]),
Config.

init_per_testcase(TestCase, Config) ->
{ok, _HooksServer} = gen_hook:start_link(),
{ok, _DomainSup} = mongoose_domain_sup:start_link(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

the same comment as for test/auth_internal_SUITE.erl

@DenysGonchar DenysGonchar merged commit 36493bc into master Aug 3, 2023
4 checks passed
@DenysGonchar DenysGonchar deleted the domain_workers_supervisor branch August 3, 2023 08:33
@chrzaszcz chrzaszcz added this to the 6.2.0 milestone Dec 11, 2023
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