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

Refactor PubSub code to prepare for RDBMS #2122

Merged
merged 4 commits into from
Nov 8, 2018

Conversation

fenek
Copy link
Member

@fenek fenek commented Nov 6, 2018

This PR refactors DB calls, mostly in node_flat, to be more efficient for future RDBMS backend. Causes more Mnesia operations but Mnesia backend should probably be rewritten or deprecated anyway.

@mongoose-im

This comment has been minimized.

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #2122 into master will decrease coverage by 1.85%.
The diff coverage is 80.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2122      +/-   ##
==========================================
- Coverage    76.9%   75.05%   -1.86%     
==========================================
  Files         323      323              
  Lines       28116    28105      -11     
==========================================
- Hits        21624    21095     -529     
- Misses       6492     7010     +518
Impacted Files Coverage Δ
src/pubsub/gen_pubsub_node.erl 10.34% <ø> (-3.45%) ⬇️
src/pubsub/node_push.erl 65.45% <100%> (-3.91%) ⬇️
src/pubsub/mod_pubsub.erl 66.02% <33.33%> (-0.07%) ⬇️
src/jid.erl 95.71% <66.66%> (-1.31%) ⬇️
src/pubsub/node_flat.erl 71.48% <85.41%> (-1.66%) ⬇️
src/pubsub/mod_pubsub_db_mnesia.erl 93.05% <92.85%> (+1.87%) ⬆️
src/mod_vcard_ldap.erl 0% <0%> (-87.87%) ⬇️
src/eldap_pool.erl 0% <0%> (-77.28%) ⬇️
src/auth/ejabberd_auth_ldap.erl 0% <0%> (-59.26%) ⬇️
src/mod_shared_roster_ldap.erl 0% <0%> (-59.04%) ⬇️
... and 20 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 7514678...6802251. Read the comment docs.

Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

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

looks fine

Copy link
Contributor

@michalwski michalwski 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 to me overall. I had only 2 comments to the code.

src/jid.erl Show resolved Hide resolved
-callback del_state(Nidx :: mod_pubsub:nodeIdx(),
UserLJID :: jid:ljid()) -> ok.
LJID :: jid:ljid()) -> ok.
Copy link
Contributor

Choose a reason for hiding this comment

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

If other callbacks expect jid:jid type, can we make this one consistent with the other and expect jid:jid as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, because it is called only in one place, where only ljid() is accessible directly so I'd have to convert it to jid(). This function will be removed anyway as soon as pubsub_item table is migrated to RDBMS as well.

@mongoose-im

This comment has been minimized.

@fenek
Copy link
Member Author

fenek commented Nov 7, 2018

@arcusfelis

I have improved the error reporting but I don't have any tests for them for now, as it is pretty urgent to move on to RDBMS implementation. However, here is a sample from provoked crash (I've added line breaks):

13:26:58.199 [error] event=pubsub_crash,details=#{
action => publish_item,
class => exit,
event => dirty_failure,
node_name => <<"leaf_J7SRi18=">>,
pubsub_host => <<"pubsub.localhost">>,
reason => undef,
stacktrace => [{mnesia_tm,non_transaction,5,[{file,"mnesia_tm.erl"},{line,746}]},{mod_pubsub_db_mnesia,dirty,2,[{file,"[...]/MongooseIM/_build/mim1/lib/mongooseim/src/pubsub/mod_pubsub_db_mnesia.erl"},{line,78}]},{mod_pubsub,publish_item,8,[{file,"[...]/MongooseIM/_build/mim1/lib/mongooseim/src/pubsub/mod_pubsub.erl"},{line,2275}]},{mod_pubsub,do_route,7,[{file,"[...]/MongooseIM/_build/mim1/lib/mongooseim/src/pubsub/mod_pubsub.erl"},{line,1009}]},{mod_pubsub,handle_info,2,[{file,"[...]/MongooseIM/_build/mim1/lib/mongooseim/src/pubsub/mod_pubsub.erl"},{line,917}]},{gen_server,try_dispatch,4,[{file,"gen_server.erl"},{line,616}]},{gen_server,handle_msg,6,[{file,"gen_server.erl"},{line,686}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,247}]}]}

@mongoose-im
Copy link
Collaborator

mongoose-im commented Nov 7, 2018

5827.1 / Erlang 19.3 / small_tests / 8df0240
Reports root / small


5827.5 / Erlang 19.3 / ldap_mnesia / 8df0240
Reports root/ big
OK: 1071 / Failed: 0 / User-skipped: 86 / Auto-skipped: 0


5827.3 / Erlang 19.3 / mysql_redis / 8df0240
Reports root/ big
OK: 2877 / Failed: 0 / User-skipped: 222 / Auto-skipped: 0


5827.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 8df0240
Reports root/ big
OK: 445 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0


5827.2 / Erlang 19.3 / internal_mnesia / 8df0240
Reports root/ big
OK: 1106 / Failed: 0 / User-skipped: 51 / Auto-skipped: 0


5827.4 / Erlang 19.3 / odbc_mssql_mnesia / 8df0240
Reports root/ big
OK: 2891 / Failed: 0 / User-skipped: 208 / Auto-skipped: 0


5827.8 / Erlang 20.0 / pgsql_mnesia / 8df0240
Reports root/ big / small
OK: 2923 / Failed: 0 / User-skipped: 176 / Auto-skipped: 0


5827.9 / Erlang 21.0 / riak_mnesia / 8df0240
Reports root/ big / small
OK: 1329 / Failed: 0 / User-skipped: 49 / Auto-skipped: 0

@michalwski
Copy link
Contributor

For some reason travis didn't change the check status from pending to success. It's all green on travis.

@michalwski michalwski merged commit bf33ae1 into master Nov 8, 2018
@michalwski michalwski deleted the pubsub-state-refactoring branch November 8, 2018 08:46
@fenek fenek added this to the 3.1.0++ milestone Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants