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

Merge Feature/cets into master #4063

Merged
merged 175 commits into from
Aug 8, 2023
Merged

Merge Feature/cets into master #4063

merged 175 commits into from
Aug 8, 2023

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jul 19, 2023

This PR checks if the CETS code from the feature branch passes the tests.
Ready for the final review and merge.

The following will be done after the merge:

  • Add missing docs for some options, e.g. component_backend
  • Update the migration guide with a description of internal_databases and the need to specify mnesia explicitly.

Use cets for stream resumption backend
Print cets tables in mongoosectl  mnesia info
Enable cets on CI
Add test for mongooseimctl mnesia info
Test ejabberd_sm_cets in ejabberd_sm_SUITE
Rename to cets
Otherwise sm_SUITE fails with already_started error
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Aug 8, 2023

small_tests_24 / small_tests / 48b9569
Reports root / small


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


small_tests_25 / small_tests / 48b9569
Reports root / small


small_tests_25_arm64 / small_tests / 48b9569
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 48b9569
Reports root/ big
OK: 2262 / Failed: 0 / User-skipped: 831 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 48b9569
Reports root/ big
OK: 4225 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 48b9569
Reports root/ big
OK: 4225 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 48b9569
Reports root/ big
OK: 2262 / Failed: 0 / User-skipped: 831 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 48b9569
Reports root/ big
OK: 4193 / Failed: 0 / User-skipped: 116 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 48b9569
Reports root/ big
OK: 4222 / Failed: 0 / User-skipped: 87 / Auto-skipped: 0


pgsql_cets_25 / pgsql_cets / 48b9569
Reports root/ big
OK: 4580 / Failed: 0 / User-skipped: 121 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 48b9569
Reports root/ big
OK: 4610 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 48b9569
Reports root/ big
OK: 4590 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 48b9569
Reports root/ big
OK: 2408 / Failed: 0 / User-skipped: 685 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 48b9569
Reports root/ big
OK: 4607 / Failed: 0 / User-skipped: 94 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 48b9569
Reports root/ big
OK: 4610 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0

@arcusfelis arcusfelis marked this pull request as ready for review August 8, 2023 11:07
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.

Ok, with such a gigantic diff, I'm not going to carefully read and verify everything changed here, some sections I'll only skim over, trusting that previous rounds of review and tests went well.
Notes:

  • I've checked documentation, seems good to me. Not extensive enough but what is there is fine.
  • I'm not verifying that everything mnesia now has a cets alternative, I trust having disabled mnesia in tests checked that already.
  • I'm not paying thorough attention to pieces like bosh, jingle, or regular muc, I've only read the main module and it seems good, I trust their respective backends and/or helper modules are cool. Though btw, if we already created a dir for muc with the _online_ helpers, we might have as well put mod_muc in there. Not important, just mentioning.
  • Changes to ejabberd_router, components, and s2s, they're amazing, thank you!

Some comments request small changes, specially the one in ejabberd_app:start/2, please fix that one 🙏🏽

@@ -80,6 +80,7 @@
{cache_tab, "1.0.30"},
{segmented_cache, "0.3.0"},
{worker_pool, "6.0.1"},
{cets, {git, "https://github.com/esl/cets.git", {branch, "main"}}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to insist in creating a hex package for this soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we will do before release (or you could try to make a package, hope the name is free... or we would have to use short eslcets)

src/ejabberd_app.erl Outdated Show resolved Hide resolved
@@ -156,12 +156,19 @@ init([]) ->
PG =
{pg,
{pg, start_link, [mim_scope]},
permanent, infinity, supervisor, [pg]},
permanent, infinity, worker, [pg]},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, this wasn't a supervisor. But now this and the one below are the only worker whose death timeout is set to infinity while all the others have a brutal_kill. Out of the scope of this PR but we should give it a thought some day (I'm already doing so 😇 )

Copy link
Contributor

Choose a reason for hiding this comment

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

rewrite it all, there is a lot of repetition in this module...

src/ejabberd_sup.erl Outdated Show resolved Hide resolved
src/mongoose_node_num.erl Outdated Show resolved Hide resolved
src/mongoose_node_num.erl Outdated Show resolved Hide resolved
src/mongoose_node_num.erl Outdated Show resolved Hide resolved
src/mongooseim.app.src Outdated Show resolved Hide resolved
@arcusfelis
Copy link
Contributor

I'm not paying thorough attention to pieces like bosh, jingle, or regular muc, I've only read the main module and it seems good, I trust their respective backends and/or helper modules are cool. Though btw, if we already created a dir for muc with the online helpers, we might have as well put mod_muc in there. Not important, just mentioning.

mod_muc.erl could be moved in a separate PR into src/muc/.

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.

LGTM 🔥🔥🔥

@mongoose-im
Copy link
Collaborator

mongoose-im commented Aug 8, 2023

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


small_tests_24 / small_tests / c5faa49
Reports root / small


small_tests_25_arm64 / small_tests / c5faa49
Reports root / small


small_tests_25 / small_tests / c5faa49
Reports root / small


ldap_mnesia_24 / ldap_mnesia / c5faa49
Reports root/ big
OK: 2262 / Failed: 0 / User-skipped: 831 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / c5faa49
Reports root/ big
OK: 2262 / Failed: 0 / User-skipped: 831 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c5faa49
Reports root/ big
OK: 4225 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / c5faa49
Reports root/ big
OK: 4193 / Failed: 0 / User-skipped: 116 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / c5faa49
Reports root/ big
OK: 4225 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / c5faa49
Reports root/ big
OK: 4222 / Failed: 0 / User-skipped: 87 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / c5faa49
Reports root/ big
OK: 4610 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


pgsql_cets_25 / pgsql_cets / c5faa49
Reports root/ big
OK: 4580 / Failed: 0 / User-skipped: 121 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / c5faa49
Reports root/ big
OK: 4590 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / c5faa49
Reports root/ big
OK: 4610 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / c5faa49
Reports root/ big
OK: 2408 / Failed: 0 / User-skipped: 685 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / c5faa49
Reports root/ big
OK: 4607 / Failed: 0 / User-skipped: 94 / Auto-skipped: 0

@NelsonVides NelsonVides merged commit 257371d into master Aug 8, 2023
4 checks passed
@NelsonVides NelsonVides deleted the feature/cets branch August 8, 2023 14:57
@arcusfelis arcusfelis changed the title Feature/cets Merge Feature/cets into master Aug 8, 2023
@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.

5 participants