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 subscriptions options implementation #2148

Merged
merged 3 commits into from
Dec 3, 2018

Conversation

fenek
Copy link
Member

@fenek fenek commented Nov 29, 2018

This PR removes pubsub_subscription module, refactors opts forms processing and integrates options storage logic into DB backends. In case of RDBMS it's only a stub for now.

@fenek fenek force-pushed the pubsub_subscriptions-refactor branch from c26552f to 2b8632a Compare November 29, 2018 17:29
@mongoose-im

This comment has been minimized.

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. I spotted one place with duplicated code.

[ {J, S, SubId} | add_jid_to_subs(RSubs, J, RStates) ].
add_jid_and_opts_to_subs([{S, SubId} | RSubs], J, RStates) ->
Opts =
case mnesia:read({pubsub_subscription, SubId}) of
Copy link
Contributor

Choose a reason for hiding this comment

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

This case and the one in line 549 look the same to me. Would it be worth to extract it to a function?

end
end,
[], Plugins),
PluginsOK = lists:foldl(pa:bind(fun init_plugin/5, Host, ServerHost, Opts), [], Plugins),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

🍺

@codecov
Copy link

codecov bot commented Nov 30, 2018

Codecov Report

Merging #2148 into master will increase coverage by 2.14%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2148      +/-   ##
==========================================
+ Coverage   75.55%   77.69%   +2.14%     
==========================================
  Files         327      327              
  Lines       28299    28230      -69     
==========================================
+ Hits        21381    21934     +553     
+ Misses       6918     6296     -622
Impacted Files Coverage Δ
src/pubsub/gen_pubsub_node.erl 100% <ø> (ø) ⬆️
src/pubsub/mod_pubsub_db.erl 100% <ø> (ø) ⬆️
src/pubsub/pubsub_form_utils.erl 2.04% <2.04%> (ø)
src/pubsub/mod_pubsub.erl 67.79% <29.54%> (+0.27%) ⬆️
src/pubsub/mod_pubsub_db_rdbms.erl 93.67% <66.66%> (+0.53%) ⬆️
src/jlib.erl 83.51% <72.72%> (+0.24%) ⬆️
src/pubsub/node_flat.erl 73.41% <80%> (ø) ⬆️
src/pubsub/mod_pubsub_db_mnesia.erl 87.65% <88.23%> (+0.4%) ⬆️
src/gen_iq_handler.erl 56.81% <0%> (-4.55%) ⬇️
src/mongoose_cluster.erl 72.36% <0%> (-2.64%) ⬇️
... and 14 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 713c33a...c1d9574. Read the comment docs.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Nov 30, 2018

5942.1 / Erlang 19.3 / small_tests / 084f51d
Reports root / small


5942.3 / Erlang 19.3 / mysql_redis / 084f51d
Reports root/ big
OK: 2924 / Failed: 0 / User-skipped: 223 / Auto-skipped: 0


5942.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 084f51d
Reports root/ big
OK: 445 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0


5942.2 / Erlang 19.3 / internal_mnesia / 084f51d
Reports root/ big
OK: 1153 / Failed: 0 / User-skipped: 52 / Auto-skipped: 0


5942.4 / Erlang 19.3 / odbc_mssql_mnesia / 084f51d
Reports root/ big
OK: 2938 / Failed: 0 / User-skipped: 209 / Auto-skipped: 0


5942.5 / Erlang 19.3 / ldap_mnesia / 084f51d
Reports root/ big
OK: 1118 / Failed: 0 / User-skipped: 87 / Auto-skipped: 0


5942.8 / Erlang 20.0 / pgsql_mnesia / 084f51d
Reports root/ big / small
OK: 2970 / Failed: 0 / User-skipped: 177 / Auto-skipped: 0


5942.9 / Erlang 21.0 / riak_mnesia / 084f51d
Reports root/ big / small
OK: 1376 / Failed: 0 / User-skipped: 50 / Auto-skipped: 0


5942.9 / Erlang 21.0 / riak_mnesia / 084f51d
Reports root/ big / small
OK: 1376 / Failed: 0 / User-skipped: 50 / Auto-skipped: 0

@michalwski michalwski merged commit cd7b3a9 into master Dec 3, 2018
@michalwski michalwski deleted the pubsub_subscriptions-refactor branch December 3, 2018 10:20
@fenek fenek added this to the 3.2.0++ milestone Jan 8, 2019
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.

3 participants