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.

Sign up for GitHub

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

Jump to bottom

Refactor async writer for MAM #3216

Merged
merged 1 commit into from
Aug 13, 2021
Merged

Refactor async writer for MAM #3216

merged 1 commit into from
Aug 13, 2021

Conversation

arcusfelis
Copy link
Contributor

This PR addresses "some old ugly code is a bit prettier now".

Proposed changes include:

  • No message_queue_len call on each archived message
  • Just do cast
  • Store messages off the heap
  • Remove modMamDropped2 metric

@mongoose-im
Copy link
Collaborator

mongoose-im commented Aug 11, 2021 •
edited
Loading

small_tests_24 / small_tests / 97052e4
Reports root / small

small_tests_22 / small_tests / 97052e4
Reports root / small

internal_mnesia_24 / internal_mnesia / 97052e4
Reports root/ big
OK: 1588 / Failed: 0 / User-skipped: 287 / Auto-skipped: 0

dynamic_domains_24 / pgsql_mnesia / 97052e4
Reports root/ big
OK: 1644 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

dynamic_domains_23 / pgsql_mnesia / 97052e4
Reports root/ big
OK: 1644 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

small_tests_23 / small_tests / 97052e4
Reports root / small

ldap_mnesia_24 / ldap_mnesia / 97052e4
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

ldap_mnesia_22 / ldap_mnesia / 97052e4
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

pgsql_mnesia_22 / pgsql_mnesia / 97052e4
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 97052e4
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 97052e4
Reports root/ big
OK: 1891 / Failed: 0 / User-skipped: 283 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / 97052e4
Reports root/ big
OK: 3075 / Failed: 1 / User-skipped: 201 / Auto-skipped: 0

mam_SUITE:rdbms_async_cache_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster 
{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok,ok]\n"}}

Report log

pgsql_mnesia_24 / pgsql_mnesia / 97052e4
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / 97052e4
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / 97052e4
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 97052e4
Reports root/ big
OK: 1738 / Failed: 0 / User-skipped: 286 / Auto-skipped: 0

@codecov
Copy link

codecov bot commented Aug 11, 2021 •
edited
Loading

Codecov Report

Merging #3216 (cf5ca0f) into master (3e9d8a8) will increase coverage by 0.11%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3216      +/-   ##
==========================================
+ Coverage   80.20%   80.31%   +0.11%     
==========================================
  Files         398      398              
  Lines       32517    32505      -12     
==========================================
+ Hits        26081    26108      +27     
+ Misses       6436     6397      -39
Impacted Files Coverage Δ
src/mam/mod_mam.erl 88.98% <ø> (ø)
src/mam/mod_mam_rdbms_async_pool_writer.erl 66.66% <87.50%> (+3.36%) ⬆️
src/mam/mod_mam_muc_rdbms_async_pool_writer.erl 71.42% <89.47%> (+4.76%) ⬆️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 71.69% <0.00%> (-1.89%) ⬇️
src/domain/mongoose_domain_loader.erl 75.47% <0.00%> (-1.89%) ⬇️
src/mod_muc_room.erl 77.18% <0.00%> (+0.05%) ⬆️
src/ejabberd_c2s.erl 89.28% <0.00%> (+0.29%) ⬆️
src/ejabberd_sm.erl 84.59% <0.00%> (+0.32%) ⬆️
src/ejabberd_s2s_out.erl 61.27% <0.00%> (+0.45%) ⬆️
src/event_pusher/mod_event_pusher_sns.erl 89.47% <0.00%> (+5.26%) ⬆️
... and 2 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 3e9d8a8...cf5ca0f. Read the comment docs.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Aug 12, 2021 •
edited
Loading

small_tests_22 / small_tests / 3f04b58
Reports root / small

dynamic_domains_24 / pgsql_mnesia / 3f04b58
Reports root/ big
OK: 1644 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

dynamic_domains_23 / pgsql_mnesia / 3f04b58
Reports root/ big
OK: 1644 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

small_tests_23 / small_tests / 3f04b58
Reports root / small

ldap_mnesia_22 / ldap_mnesia / 3f04b58
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / 3f04b58
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 3f04b58
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

pgsql_mnesia_22 / pgsql_mnesia / 3f04b58
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 3f04b58
Reports root/ big
OK: 1891 / Failed: 0 / User-skipped: 283 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 3f04b58
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / 3f04b58
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / 3f04b58
Reports root/ big
OK: 3069 / Failed: 0 / User-skipped: 201 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / 3f04b58
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 3f04b58
Reports root/ big
OK: 1738 / Failed: 0 / User-skipped: 286 / Auto-skipped: 0

@arcusfelis
Refactor async writer for MAM
cf5ca0f 
No message_queue_len call on each archived message
Just do cast
Store messages off the heap
Remove modMamDropped2 metric
@mongoose-im
Copy link
Collaborator

mongoose-im commented Aug 13, 2021 •
edited
Loading

small_tests_24 / small_tests / cf5ca0f
Reports root / small

internal_mnesia_24 / internal_mnesia / cf5ca0f
Reports root/ big
OK: 1588 / Failed: 0 / User-skipped: 287 / Auto-skipped: 0

dynamic_domains_24 / pgsql_mnesia / cf5ca0f
Reports root/ big
OK: 1644 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

small_tests_22 / small_tests / cf5ca0f
Reports root / small

dynamic_domains_23 / pgsql_mnesia / cf5ca0f
Reports root/ big
OK: 1644 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

small_tests_23 / small_tests / cf5ca0f
Reports root / small

ldap_mnesia_24 / ldap_mnesia / cf5ca0f
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

ldap_mnesia_22 / ldap_mnesia / cf5ca0f
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / cf5ca0f
Reports root/ big
OK: 1515 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / cf5ca0f
Reports root/ big
OK: 1891 / Failed: 0 / User-skipped: 283 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / cf5ca0f
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / cf5ca0f
Reports root/ big
OK: 3069 / Failed: 0 / User-skipped: 201 / Auto-skipped: 0

pgsql_mnesia_22 / pgsql_mnesia / cf5ca0f
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / cf5ca0f
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / cf5ca0f
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / cf5ca0f
Reports root/ big
OK: 1738 / Failed: 0 / User-skipped: 286 / Auto-skipped: 0

chrzaszcz
chrzaszcz approved these changes Aug 13, 2021
View reviewed changes
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks good! Load tests show a slight increase in performance and the batches are not lost somehow, maybe because of the removed spawn_monitor.

@chrzaszcz chrzaszcz merged commit 51bc7f0 into master Aug 13, 2021
@chrzaszcz chrzaszcz deleted the mu-refactor-async-writer branch August 13, 2021 15:45
NelsonVides
NelsonVides reviewed Aug 13, 2021
View reviewed changes
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.

I like this one too very much as well. The off-heap strategy is a brilliant tiny detail, and nice removing of more spawns 👌🏽

@Premwoik Premwoik added this to the 5.0.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NelsonVides NelsonVides NelsonVides left review comments

@chrzaszcz chrzaszcz chrzaszcz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@arcusfelis @mongoose-im @chrzaszcz @NelsonVides @Premwoik