Hosttypyfy wpool #3170

arcusfelis · 2021-07-02T16:57:42Z

Proposed changes include:

HostTypes in wpool
Better specs in wpool

codecov · 2021-07-02T17:23:34Z

Codecov Report

Merging #3170 (318756d) into master (2a6381f) will decrease coverage by 20.58%.
The diff coverage is 78.78%.

❗ Current head 318756d differs from pull request most recent head 91f57d2. Consider uploading reports for the commit 91f57d2 to get more accurate results

@@             Coverage Diff             @@
##           master    #3170       +/-   ##
===========================================
- Coverage   79.92%   59.33%   -20.59%     
===========================================
  Files         396      396               
  Lines       32335    32347       +12     
===========================================
- Hits        25843    19194     -6649     
- Misses       6492    13153     +6661

Impacted Files	Coverage Δ
src/wpool/mongoose_wpool_cassandra.erl	`0.00% <0.00%> (-95.66%)`	⬇️
src/wpool/mongoose_wpool_elastic.erl	`0.00% <0.00%> (-90.91%)`	⬇️
src/wpool/mongoose_wpool_mgr.erl	`76.05% <ø> (ø)`
src/wpool/mongoose_wpool_rdbms.erl	`0.00% <0.00%> (-79.17%)`	⬇️
src/wpool/mongoose_wpool_sup.erl	`100.00% <ø> (ø)`
src/wpool/mongoose_wpool.erl	`75.72% <78.94%> (-9.28%)`	⬇️
src/event_pusher/mod_event_pusher_push.erl	`94.73% <100.00%> (+0.17%)`	⬆️
src/event_pusher/mod_event_pusher_sns.erl	`89.47% <100.00%> (+0.43%)`	⬆️
src/mod_push_service_mongoosepush.erl	`83.60% <100.00%> (+0.55%)`	⬆️
src/wpool/mongoose_wpool_generic.erl	`100.00% <100.00%> (ø)`
... and 156 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 2a6381f...91f57d2. Read the comment docs.

mongoose-im · 2021-07-06T13:28:07Z

small_tests_21_3 / small_tests / 22c3d6e
Reports root / small

dynamic_domains / pgsql_mnesia / 22c3d6e
Reports root/ big
OK: 1565 / Failed: 0 / User-skipped: 121 / Auto-skipped: 0

small_tests_22 / small_tests / 22c3d6e
Reports root / small

small_tests_23 / small_tests / 22c3d6e
Reports root / small

ldap_mnesia_21 / ldap_mnesia / 22c3d6e
Reports root/ big
OK: 1514 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0

internal_mnesia / internal_mnesia / 22c3d6e
Reports root/ big
OK: 1586 / Failed: 1 / User-skipped: 286 / Auto-skipped: 0

dynamic_domains_SUITE:with_mod_dynamic_domains_test:packet_handling_for_subdomain

{error,
  {{badrpc,
     {'EXIT',
       {timeout,
         [{meck_proc,wait,6,
            [{file,
               "/home/circleci/app/_build/default/lib/meck/src/meck_proc.erl"},
             {line,171}]},
          {meck,wait,5,[]}]}}},
   [{distributed_helper,rpc,
      [#{node => mongooseim@localhost},
       meck,wait,
       [3,mod_dynamic_domains_test,process_packet,5,500]],
      [{file,"/home/circleci/app/big_tests/tests/distributed_helper.erl"},
       {line,117}]},
    {dynamic_domains_SUITE,'-packet_handling_for_subdomain/1-fun-3-',1,
      [{file,
         "/home/circleci/app/big_tests/tests/dynamic_domains_SUITE.erl"},
       {line,113}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
       {line,72}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1754}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1263}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1195}]}]}}

Report log

ldap_mnesia_23 / ldap_mnesia / 22c3d6e
Reports root/ big
OK: 1514 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0

ldap_mnesia_22 / ldap_mnesia / 22c3d6e
Reports root/ big
OK: 1514 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0

elasticsearch_and_cassandra / elasticsearch_and_cassandra_mnesia / 22c3d6e
Reports root/ big
OK: 1890 / Failed: 0 / User-skipped: 282 / Auto-skipped: 0

pgsql_mnesia / pgsql_mnesia / 22c3d6e
Reports root/ big
OK: 3083 / Failed: 0 / User-skipped: 184 / Auto-skipped: 1

mysql_redis / mysql_redis / 22c3d6e
Reports root/ big
OK: 3067 / Failed: 0 / User-skipped: 201 / Auto-skipped: 0

mssql_mnesia / odbc_mssql_mnesia / 22c3d6e
Reports root/ big
OK: 3082 / Failed: 0 / User-skipped: 184 / Auto-skipped: 2

riak_mnesia / riak_mnesia / 22c3d6e
Reports root/ big
OK: 1737 / Failed: 0 / User-skipped: 285 / Auto-skipped: 0

internal_mnesia / internal_mnesia / 22c3d6e
Reports root/ big
OK: 1587 / Failed: 0 / User-skipped: 286 / Auto-skipped: 0

pgsql_mnesia / pgsql_mnesia / 22c3d6e
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

mssql_mnesia / odbc_mssql_mnesia / 22c3d6e
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

DenysGonchar · 2021-07-09T15:24:57Z

rebar.config

@@ -71,7 +73,7 @@
  {cpool, "0.1.0"},
  {observer_cli, "1.5.4"},
  {nkpacket, {git, "https://github.com/michalwski/nkpacket.git", {ref, "f7c5349"}}},
-  {nksip, {git, "https://github.com/NetComposer/nksip.git", {ref, "1a29ef3"}}},
+  {nksip, {git, "https://github.com/arcusfelis/nksip.git", {branch, "mu-fix-types-1a29ef3"}}},


please submit PR with these fixes to the original repo.

We use nksip from 5 years ago:

commit 1a29ef3e112ee0a8d8ac53bd6963ccc8f7879343 Author: Carlos Gonzalez <carlosj.gf@gmail.com> Date: Fri Nov 4 20:13:40 2016 +0100 Fixed license

We would be a bit late with this PR

DenysGonchar · 2021-07-09T15:31:37Z

src/cassandra/mongoose_cqerl_helper.erl

+
+%% Used to supress opaque check of dialyzer
+-spec identity(any()) -> any().
+identity(X) -> X.


I don't really understand why we need it. please, illustrate it on some example?

without it you will get error that opaque type is used in mim code, while tuple is expected.

DenysGonchar · 2021-07-09T15:41:19Z

src/jingle_sip/mod_jingle_sip.erl

@@ -284,6 +271,28 @@ translate_to_sip(<<"session-terminate">>, Jingle, Acc) ->
            {error, item_not_found}
    end.

+%% There is some mess with types hapenning in nksip_uac:invite/3


didn't you fix it in your fork?

nooo, there are still weird types in nksip.
I've fixed one or two errors only.

DenysGonchar · 2021-07-09T15:43:29Z

src/mam/mod_mam_utils.erl

@@ -138,6 +138,8 @@ rsm_ns_binary() -> <<"http://jabber.org/protocol/rsm">>.
 -type archive_behaviour() :: mod_mam:archive_behaviour().
 -type archive_behaviour_bin() :: binary(). % `<<"roster">> | <<"always">> | <<"never">>'.

+%% calendar:rfc3339_string() type is not exported
+-type rfc3339_string() :: [byte(), ...].


Should we put this type in some generic module (e.g. mongooseim)?

DenysGonchar · 2021-07-12T00:37:09Z

src/mod_stream_management.erl

-type smid() :: base64:ascii_binary().
+
+%% base64:ascii_binary() type is not exported
+-type ascii_binary() :: binary().


the same question, should we put this type in some generic module (e.g. mongooseim)?

DenysGonchar · 2021-07-12T00:50:07Z

src/mongoose_redis.erl

@@ -7,6 +7,7 @@
 -module(mongoose_redis).
 -author("bartlomiej.gorny@erlang-solutions.com").
 -include("mongoose.hrl").
+-include_lib("eredis/include/eredis.hrl").


is return_value() defined in eredis.hrl, it seems to be a wrong approach

say it to eredis authors

DenysGonchar · 2021-07-12T01:18:50Z

rebar.config

@@ -178,7 +180,24 @@
   {override, jwerl, [{plugins, [rebar3_elixir, rebar3_hex]}]}
 ]}.

-{dialyzer, [{plt_extra_apps, [jid, cowboy, lasse, p1_utils, ranch, gen_fsm_compat, epgsql]}]}.
+{dialyzer, [


great job, but probably it's better to have it in a separate PR. it's just a wish for the future, no need to make a split of this one.

DenysGonchar · 2021-07-12T01:23:02Z

src/jlib.erl

@@ -78,12 +78,16 @@
 -type rsm_in() :: #rsm_in{}.
 -type rsm_out() :: #rsm_out{}.

+%% calendar:rfc3339_string() type is not exported
+-type rfc3339_string() :: [byte(), ...].


Should we put this type in some generic module (e.g. mongooseim)?

DenysGonchar

PR looks very good, I'd like to recheck just a few minor things before merging it.

DenysGonchar · 2021-07-12T06:56:41Z

Also that would be nice to run some integration tests with host-type specific pools.

Looks too similar to HostType.

mongoose-im · 2021-07-12T12:39:52Z

small_tests_21_3 / small_tests / 91f57d2
Reports root / small

small_tests_22 / small_tests / 91f57d2
Reports root / small

dynamic_domains / pgsql_mnesia / 91f57d2
Reports root/ big
OK: 1585 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

small_tests_23 / small_tests / 91f57d2
Reports root / small

ldap_mnesia_21 / ldap_mnesia / 91f57d2
Reports root/ big
OK: 1514 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0

ldap_mnesia_22 / ldap_mnesia / 91f57d2
Reports root/ big
OK: 1514 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0

internal_mnesia / internal_mnesia / 91f57d2
Reports root/ big
OK: 1587 / Failed: 0 / User-skipped: 286 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 91f57d2
Reports root/ big
OK: 1514 / Failed: 0 / User-skipped: 359 / Auto-skipped: 0

elasticsearch_and_cassandra / elasticsearch_and_cassandra_mnesia / 91f57d2
Reports root/ big
OK: 1890 / Failed: 0 / User-skipped: 282 / Auto-skipped: 0

mssql_mnesia / odbc_mssql_mnesia / 91f57d2
Reports root/ big
OK: 3083 / Failed: 0 / User-skipped: 184 / Auto-skipped: 1

pgsql_mnesia / pgsql_mnesia / 91f57d2
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

mysql_redis / mysql_redis / 91f57d2
Reports root/ big
OK: 3075 / Failed: 1 / User-skipped: 201 / Auto-skipped: 0

connect_SUITE:just_tls:feature_order:auth_compression_bind_session

{error,
  {{badmatch,
     {error,
       {connection_step_failed,
         {{escalus_session,maybe_use_compression},
          {client,
            <<"secure_joe_auth_compression_bind_session_48.697942@localhost">>,
            escalus_tcp,<0.13384.0>,undefined,
            [{username,
               <<"secure_joe_auth_compression_bind_session_48.697942">>},
             {server,<<"localhost">>},
             {password,<<"break_me">>},
             {compression,<<"zlib">>},
             {starttls,required},
             {host,<<"localhost">>},
             {stream_id,<<"60841b22773b84ab">>}]},
          [{compression,[<<"zlib">>]},
           {starttls,false},
           {stream_management,true},
           {advanced_message_processing,true},
           {client_state_indication,false},
           {sasl_mechanisms,[<<"SCRAM-SHA-256">>,<<"PLAIN">>]},
           {caps,undefined}]},
         {timeout,stream_start}}}},
   [{connect_SUITE,auth_compression_bind_session,1,
      [{file,"/home/circleci/app/big_tests/tests/connect_SUITE.erl"},
       {line,517}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1754}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1263}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1195}]}]}}

Report log

riak_mnesia / riak_mnesia / 91f57d2
Reports root/ big
OK: 1737 / Failed: 0 / User-skipped: 285 / Auto-skipped: 0

mssql_mnesia / odbc_mssql_mnesia / 91f57d2
Reports root/ big
OK: 3084 / Failed: 0 / User-skipped: 184 / Auto-skipped: 0

chrzaszcz

Nice, I like the new types, good work!

big_tests/test.config

chrzaszcz · 2021-07-13T15:14:37Z

src/event_pusher/mod_event_pusher_push.erl

@@ -74,26 +74,31 @@
 %%--------------------------------------------------------------------
 %% gen_mod callbacks
 %%--------------------------------------------------------------------
-spec start(Host :: jid:server(), Opts :: list()) -> any().
+-spec start(HostType :: mongooseim:host_type(), Opts :: gen_mod:module_opts()) -> any().


Minor, I think that variable names should agree between the spec and the implementation. We could either change to HostType or leave Host in both places.

arcusfelis force-pushed the mu-hosttype-wpool branch from 7c6f4f2 to 22c3d6e Compare July 6, 2021 13:09

DenysGonchar reviewed Jul 9, 2021

View reviewed changes

DenysGonchar reviewed Jul 12, 2021

View reviewed changes

DenysGonchar approved these changes Jul 12, 2021

View reviewed changes

arcusfelis added the WIP 🚧 label Jul 12, 2021

arcusfelis added 5 commits July 12, 2021 14:16

Rename mongoose_wpool:host() to scope()

e73cefc

Rename Type in the pool code to PoolType

f2de4a7

Looks too similar to HostType.

Fix some dialyzer warnings

9dbd9fb

Use HostType in the backend wpool modules

06f1b8f

Fix wpool tests

91f57d2

arcusfelis force-pushed the mu-hosttype-wpool branch from 22c3d6e to 91f57d2 Compare July 12, 2021 12:18

This comment has been minimized.

Sign in to view

arcusfelis force-pushed the mu-hosttype-wpool branch from 318756d to 91f57d2 Compare July 13, 2021 10:30

chrzaszcz approved these changes Jul 13, 2021

View reviewed changes

chrzaszcz merged commit f86f1d1 into master Jul 13, 2021

chrzaszcz deleted the mu-hosttype-wpool branch July 13, 2021 15:36

Premwoik added this to the 5.0.0 milestone Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hosttypyfy wpool #3170

Hosttypyfy wpool #3170

arcusfelis commented Jul 2, 2021 •

edited

Loading

codecov bot commented Jul 2, 2021 •

edited

Loading

mongoose-im commented Jul 6, 2021 •

edited

Loading

DenysGonchar Jul 9, 2021

arcusfelis Jul 12, 2021 •

edited

Loading

DenysGonchar Jul 9, 2021

arcusfelis Jul 12, 2021

DenysGonchar Jul 9, 2021

arcusfelis Jul 12, 2021

DenysGonchar Jul 9, 2021

DenysGonchar Jul 12, 2021

DenysGonchar Jul 12, 2021

arcusfelis Jul 12, 2021

DenysGonchar Jul 12, 2021

DenysGonchar Jul 12, 2021

DenysGonchar left a comment

DenysGonchar commented Jul 12, 2021

mongoose-im commented Jul 12, 2021 •

edited

Loading

This comment has been minimized.

chrzaszcz left a comment

chrzaszcz Jul 13, 2021

Hosttypyfy wpool #3170

Hosttypyfy wpool #3170

Conversation

arcusfelis commented Jul 2, 2021 • edited Loading

codecov bot commented Jul 2, 2021 • edited Loading

Codecov Report

mongoose-im commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

arcusfelis Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar left a comment

Choose a reason for hiding this comment

DenysGonchar commented Jul 12, 2021

mongoose-im commented Jul 12, 2021 • edited Loading

This comment has been minimized.

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis commented Jul 2, 2021 •

edited

Loading

codecov bot commented Jul 2, 2021 •

edited

Loading

mongoose-im commented Jul 6, 2021 •

edited

Loading

arcusfelis Jul 12, 2021 •

edited

Loading

mongoose-im commented Jul 12, 2021 •

edited

Loading