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

Remove the ETS table from gen_mod #3484

Merged
merged 7 commits into from
Jan 5, 2022
Merged

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Dec 31, 2021

Before:

  • Module options were stored in two places: mongoose_config (persistent terms) and gen_mod (ETS).
  • API for getting module options and loaded modules was querying the ETS table.
  • When a module was started/stopped by big tests, the check for already started/stopped module was duplicated: one in mongoose_modules and one in gen_mod.
  • You shouldn't call gen_mod directly to start a module in the shell, but it was still possible.

After:

  • Module options are stored only in mongoose_config.
  • API for getting module options and loaded modules is querying the persistent terms.
  • When a module is started/stopped by big tests, the check for already started/stopped module is done only in mongoose_modules.
  • It is not possible to start a module in the shell with gen_mod because the config is missing, you should use mongoose_modules instead.

Other changes:

  • Better test coverage for mongoose_modules and gen_mod API.

@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #3484 (e448b7c) into master (1262811) will increase coverage by 0.02%.
The diff coverage is 71.42%.

❗ Current head e448b7c differs from pull request most recent head fc28e62. Consider uploading reports for the commit fc28e62 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3484      +/-   ##
==========================================
+ Coverage   80.92%   80.95%   +0.02%     
==========================================
  Files         416      416              
  Lines       32352    32333      -19     
==========================================
- Hits        26180    26174       -6     
+ Misses       6172     6159      -13     
Impacted Files Coverage Δ
src/ejabberd_app.erl 94.66% <ø> (-0.08%) ⬇️
src/gen_mod.erl 68.51% <66.66%> (+1.05%) ⬆️
src/mongoose_modules.erl 100.00% <100.00%> (ø)
src/http_upload/mod_http_upload.erl 94.62% <0.00%> (-2.16%) ⬇️
src/logger/mongoose_log_filter.erl 78.08% <0.00%> (-1.37%) ⬇️
src/ejabberd_c2s.erl 89.18% <0.00%> (+0.29%) ⬆️
src/mam/mod_mam_elasticsearch_arch.erl 86.84% <0.00%> (+1.75%) ⬆️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 73.58% <0.00%> (+1.88%) ⬆️
src/config/mongoose_config.erl 97.22% <0.00%> (+2.77%) ⬆️
src/elasticsearch/mongoose_elasticsearch.erl 84.61% <0.00%> (+7.69%) ⬆️

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 1262811...fc28e62. Read the comment docs.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@chrzaszcz chrzaszcz force-pushed the module-config-map branch 3 times, most recently from d1f9866 to cd245ff Compare January 3, 2022 08:47
Base automatically changed from module-config-map to master January 3, 2022 16:15
@chrzaszcz chrzaszcz changed the base branch from master to unfold-module-opts January 4, 2022 09:12
@mongoose-im

This comment has been minimized.

Base automatically changed from unfold-module-opts to master January 4, 2022 09:23
Motivation: data duplication
  - module configuration is already stored in mongoose_config
  - if a module is mongoose_config, it is running (also in tests),
    because mongoose_modules takes care of this consistency.

Main changes apart form the ETS table removal:
  - Removed check for already_started - it is already done by
    mongoose_modules and gen_mod cannot be used directly anyway.
  - Added assertions for is_loaded - to be sure that the state is
    consistent at this point.
  - 'loaded_modules_with_opts' returns a map
- There is no 'already_started' error anymore
- start/stop_module throw errors if the module is not loaded
- start/stop_module logs module errors and re-throws them
@mongoose-im

This comment has been minimized.

@chrzaszcz chrzaszcz changed the title Experimental: remove ejabberd_modules Remove the ETS table from gen_mod Jan 4, 2022
- replace_modules accepts a map instead of a list for less conversions.
- start/stop_module results are not matched as they can only return
  {ok, term()) and ok, respectively.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 4, 2022

small_tests_24 / small_tests / fc28e62
Reports root / small


small_tests_23 / small_tests / fc28e62
Reports root / small


dynamic_domains_mysql_redis_24 / mysql_redis / fc28e62
Reports root/ big
OK: 2671 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / fc28e62
Reports root/ big
OK: 2688 / Failed: 0 / User-skipped: 230 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / fc28e62
Reports root/ big
OK: 2688 / Failed: 0 / User-skipped: 230 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / fc28e62
Reports root/ big
OK: 2688 / Failed: 0 / User-skipped: 230 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / fc28e62
Reports root/ big
OK: 1501 / Failed: 0 / User-skipped: 388 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / fc28e62
Reports root/ big
OK: 1501 / Failed: 0 / User-skipped: 388 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / fc28e62
Reports root/ big
OK: 1553 / Failed: 1 / User-skipped: 347 / Auto-skipped: 0

pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription
{error,
  {{badmatch,
     [{xmlel,<<"message">>,
        [{<<"from">>,
        <<"alice_unsubscribe_after_presence_unsubscription_965@localhost">>},
         {<<"to">>,
        <<"bob_unsubscribe_after_presence_unsubscription_965@localhost/res1">>},
         {<<"type">>,<<"headline">>}],
        [{xmlel,<<"event">>,
           [{<<"xmlns">>,
           <<"http://jabber.org/protocol/pubsub#event">>}],
           [{xmlel,<<"items">>,
            [{<<"node">>,<<"/zgJEyw+e5/DPEBfASg89w==">>}],
            [{xmlel,<<"item">>,
               [{<<"id">>,<<"salmon">>}],
               [{xmlel,<<"entry">>,
                  [{<<"xmlns">>,
                  <<"http://www.w3.org/2005/Atom">>}],
                  []}]}]}]},
         {xmlel,<<"headers">>,
           [{<<"xmlns">>,<<"http://jabber.org/protocol/shim">>}],
           []}]}]},
   [{pep_SUITE,'-unsubscribe_after_presence_unsubscription/1-fun-0-',2,
      [{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
       {line,384}]},
    {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,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


pgsql_mnesia_23 / pgsql_mnesia / fc28e62
Reports root/ big
OK: 3075 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / fc28e62
Reports root/ big
OK: 3075 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / fc28e62
Reports root/ big
OK: 3075 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / fc28e62
Reports root/ big
OK: 1835 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / fc28e62
Reports root/ big
OK: 3070 / Failed: 0 / User-skipped: 244 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / fc28e62
Reports root/ big
OK: 1681 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review January 4, 2022 14:34
Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

@NelsonVides NelsonVides merged commit 42a28d4 into master Jan 5, 2022
@NelsonVides NelsonVides deleted the without-ejabberd-modules branch January 5, 2022 08:45
@Premwoik Premwoik added this to the 5.1.0 milestone May 25, 2022
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.

None yet

4 participants