-
Notifications
You must be signed in to change notification settings - Fork 428
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
mod_private without dynamic modules #3323
mod_private without dynamic modules #3323
Conversation
Codecov Report
@@ Coverage Diff @@
## without-dynamic-backend-modules #3323 +/- ##
===================================================================
+ Coverage 80.69% 80.76% +0.07%
===================================================================
Files 397 399 +2
Lines 32434 32472 +38
===================================================================
+ Hits 26171 26227 +56
+ Misses 6263 6245 -18
Continue to review full report at Codecov.
|
small_tests_24 / small_tests / 07db703 internal_mnesia_24 / internal_mnesia / 07db703 small_tests_23 / small_tests / 07db703 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"[email protected]\">>},\n {<<\"to\">>,\n <<\"[email protected]/res1\">>},\n {<<\"id\">>,<<\"f1c3c1eeb72df1febc92cc6a902e754e\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"[email protected]\">>},\n {<<\"to\">>,\n <<\"[email protected]/res1\">>},\n {<<\"id\">>,<<\"1bd3d9227990152cc3d408290f6cd323\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} dynamic_domains_mysql_redis_24 / mysql_redis / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"[email protected]\">>},\n {<<\"to\">>,\n <<\"[email protected]/res1\">>},\n {<<\"id\">>,<<\"d6b98b7b616334aec2cd1c4f2b13b692\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} ldap_mnesia_23 / ldap_mnesia / 07db703 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"[email protected]\">>},\n {<<\"to\">>,\n <<\"[email protected]/res1\">>},\n {<<\"id\">>,<<\"8c7bc43640d4363ca890dd1b161cff23\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} ldap_mnesia_24 / ldap_mnesia / 07db703 pgsql_mnesia_23 / pgsql_mnesia / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"alicE_private_removal_87.130667@localhost\">>},\n {<<\"to\">>,\n <<\"alicE_private_removal_87.130667@localhost/res1\">>},\n {<<\"id\">>,<<\"611b336c5538d852070c01fad7f87248\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} pgsql_mnesia_24 / pgsql_mnesia / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"alicE_private_removal_2.265201@localhost\">>},\n {<<\"to\">>,\n <<\"alicE_private_removal_2.265201@localhost/res1\">>},\n {<<\"id\">>,<<\"dcc1a0d2f0874fe437cc65aeec977f90\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} mssql_mnesia_24 / odbc_mssql_mnesia / 07db703 service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join{error,{test_case_failed,{[<<"example1.com">>,<<"example2.com">>,
<<"example3.com">>],
[<<"example1.com">>,<<"example2.com">>,
<<"example3.com">>,<<"example4.com">>]}}} domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"alicE_private_removal_18.308091@localhost\">>},\n {<<\"to\">>,\n <<\"alicE_private_removal_18.308091@localhost/res1\">>},\n {<<\"id\">>,<<\"31d1396dcceeedb9b0d96923b74258a3\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 07db703 mysql_redis_24 / mysql_redis / 07db703 domain_removal_SUITE:private_removal:private_removal{error,{test_case_failed,"assert_equal_extra(<< >>, Val2)\n\tExpected <<>>\n\tValue <<\"banana\">>\n\tstanza = {xmlel,<<\"iq\">>,\n [{<<\"from\">>,\n <<\"alicE_private_removal_90.544560@localhost\">>},\n {<<\"to\">>,\n <<\"alicE_private_removal_90.544560@localhost/res1\">>},\n {<<\"id\">>,<<\"2ee10edfead16ba7cdfe64b6a0374f26\">>},\n {<<\"type\">>,<<\"result\">>}],\n [{xmlel,<<\"query\">>,\n [{<<\"xmlns\">>,<<\"jabber:iq:private\">>}],\n [{xmlel,<<\"my_element\">>,\n [{<<\"xmlns\">>,<<\"alice:private:ns\">>}],\n [{xmlcdata,<<\"banana\">>}]}]}]}\n"}} riak_mnesia_24 / riak_mnesia / 07db703 |
small_tests_24 / small_tests / df8ef72 internal_mnesia_24 / internal_mnesia / df8ef72 small_tests_23 / small_tests / df8ef72 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / df8ef72 dynamic_domains_mysql_redis_24 / mysql_redis / df8ef72 ldap_mnesia_24 / ldap_mnesia / df8ef72 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / df8ef72 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / df8ef72 pgsql_mnesia_24 / pgsql_mnesia / df8ef72 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / df8ef72 ldap_mnesia_23 / ldap_mnesia / df8ef72 mysql_redis_24 / mysql_redis / df8ef72 mssql_mnesia_24 / odbc_mssql_mnesia / df8ef72 jingle_SUITE:all:resp_4xx_from_sip_proxy_results_in_session_terminate{error,
{{assertion_failed,assert,is_iq_result,
{xmlel,<<"iq">>,
[{<<"from">>,<<"error.480@localhost">>},
{<<"to">>,
<<"alice_resp_4xx_from_sip_proxy_results_in_session_terminate_17.936775@localhost/res1">>},
{<<"id">>,<<"c85d8b76-051e-48e5-a743-dfaf29fe083f">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"jingle">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:1">>},
{<<"action">>,<<"session-terminate">>},
{<<"sid">>,<<"1dc8a2fa-034c-4e57-998a-efc2b01c59f0">>}],
[{xmlel,<<"reason">>,[],
[{xmlel,<<"general-error">>,[],[]},
{xmlel,<<"sip-error">>,
[{<<"code">>,<<"480">>}],
[{xmlcdata,<<"Temporarily Unavailable">>}]}]}]}]},
"<iq from='error.480@localhost' to='alice_resp_4xx_from_sip_proxy_results_in_session_terminate_17.936775@localhost/res1' id='c85d8b76-051e-48e5-a743-dfaf29fe083f' type='set'><jingle xmlns='urn:xmpp:jingle:1' action='session-terminate' sid='1dc8a2fa-034c-4e57-998a-efc2b01c59f0'><reason><general-error/><sip-error code='480'>Temporarily Unavailable</sip-error></reason></jingle></iq>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{jingle_SUITE,send_initiate_and_wait_for_first_iq_set,2,
[{file,"/home/circleci/app/big_tests/tests/jingle_SUITE.erl"},
{line,395}]},
{jingle_SUITE,
'-res... pgsql_mnesia_23 / pgsql_mnesia / df8ef72 riak_mnesia_24 / riak_mnesia / df8ef72 |
There was a problem hiding this 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 think we could proceed with other modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't see this ready to be merged at all, but I don't want to block it either 😕
src/backend_module.erl
Outdated
|
||
call(HostType, Module, Op, Args) -> | ||
Backend = gen_mod:get_backend_module(HostType, Module), | ||
erlang:apply(Backend, Op, Args). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunately the most expensive way to call functions in erlang https://erlang.org/doc/efficiency_guide/functions.html#function-calls
That's precisely why @DenysGonchar made the effort for hooks to register properly performant callbacks
Line 235 in 2c4bd44
check_hook_function(Function) when is_function(Function, 3) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code you mentioned above is using erlang:apply
so it is not more performant.
I think the point or this PR is to have a simple baseline. I don't think it will be noticeably slower (the metrics are very slow compared to any erlang:apply
). Let's keep this in a feature branch then if we are unsure about performance and merge only when we load test it.
We could optimize this code later and the module code could stay as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the hook we have just one handler function, but backend is a behavior implementation module. I'm not sure if we can make it somehow differently here, and in the end it's done in the same way in OTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this PR's destination to a feature branch in which we will eliminate the dynamically compiled modules and maybe optimize if load test show it's slower.
src/mod_private_backend.erl
Outdated
remove_domain/2]). | ||
|
||
-author('[email protected]'). | ||
-behaviour(mod_private). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this module should define the behaviour, not implement it. The backends themselves would implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That behaviour is defined in mod_private.
mod_private_backend implements this behaviour too. It just proxies it :)
We could move the behaviour definition from mod_private though ;)
Regarding the efficiency guide, I implemented the following:
I called it in the shell:
There were no measurable differences even for 10 million quick calls (!) and in the backend module we are calling a heavy function just once. For me getting rid of erlang:apply here would be premature optimization at its finest. |
Shell code is not compiled code. Try using Benchee for better statistics and proper compilation. |
The point was to show how cheap all these calls are, not to measure the tiny differences. The lack of difference was only the icing on the cake for me. Anyway, I tried to measure the difference without passing any function references from the shell and got the same results even if the function called was |
Being honest, I don't see any real reason to discuss it. For calling the behavior implementation, you can not avoid |
-module(benc).
-export([get_back/1]).
-export([get_pers/1]).
-export([get_pers_no_def/1]).
get_back(N) ->
timer:tc(fun() -> get_back0(N) end).
get_back0(0) ->
ok;
get_back0(N) ->
gen_mod:get_backend_module(<<"localhost">>, mod_private),
get_back0(N-1).
get_pers(N) ->
timer:tc(fun() -> get_pers0(N) end).
get_pers0(0) ->
ok;
get_pers0(N) ->
persistent_term:get({backend_module, mod_private, <<"localhost">>}, mnesia),
get_pers0(N-1).
get_pers_no_def(N) ->
persistent_term:put({backend_module2, mod_private, <<"localhost">>}, mnesia),
timer:tc(fun() -> get_pers_no_def0(N) end).
get_pers_no_def0(0) ->
ok;
get_pers_no_def0(N) ->
persistent_term:get({backend_module2, mod_private, <<"localhost">>}),
get_pers_no_def0(N-1).
|
Also, made mod_private_backend module
small_tests_24 / small_tests / 3ec796d |
3ec796d
to
ea36f02
Compare
small_tests_24 / small_tests / ea36f02 internal_mnesia_24 / internal_mnesia / ea36f02 sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_send_message{error,
{{assertion_failed,assert,is_presence,
{xmlel,<<"message">>,
[{<<"from">>,
<<"bOb_resume_session_state_send_message_49.952849@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alicE_resume_session_state_send_message_49.978429@localhost">>},
{<<"xml:lang">>,<<"en">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-3">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>},
{<<"stamp">>,<<"2021-10-11T12:04:10.032966Z">>},
{<<"from">>,<<"localhost">>}],
[{xmlcdata,<<"Offline Storage">>}]}]},
"<message from='bOb_resume_session_state_send_message_49.952849@localhost/escalus-default-resource' to='alicE_resume_session_state_send_message_49.978429@localhost' xml:lang='en' type='chat'><body>msg-3</body><delay xmlns='urn:xmpp:delay' stamp='2021-10-11T12:04:10.032966Z' from='localhost'>Offline Storage</delay></message>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{sm_SUITE,resume_session_state_send_message,1,
[{file,"/home/circleci/app/big_tests/tests/sm_SUITE.erl"},
{line,727}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_serve... |
ea36f02
to
22cc73c
Compare
small_tests_24 / small_tests / 22cc73c internal_mnesia_24 / internal_mnesia / 22cc73c small_tests_23 / small_tests / 22cc73c dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 22cc73c rest_client_SUITE:muc:messages_can_be_paginated_in_room{error,
{{assertion_failed,assert,is_chat_message,
[<<"967b220b">>],
{xmlel,<<"message">>,
[{<<"to">>,<<"[email protected]">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"c7653654">>}]}]},
"<message to='[email protected]' type='chat'><body>c7653654</body></message>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{rest_client_SUITE,assert_room_messages,2,
[{file,"/home/circleci/app/big_tests/tests/rest_client_SUITE.erl"},
{line,772}]},
{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,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} ldap_mnesia_24 / ldap_mnesia / 22cc73c ldap_mnesia_23 / ldap_mnesia / 22cc73c dynamic_domains_mysql_redis_24 / mysql_redis / 22cc73c dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 22cc73c dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 22cc73c service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join{error,{test_case_failed,{[<<"example1.com">>,<<"example2.com">>,
<<"example3.com">>],
[<<"example1.com">>,<<"example2.com">>,
<<"example3.com">>,<<"example4.com">>]}}} pgsql_mnesia_24 / pgsql_mnesia / 22cc73c amp_big_SUITE:mam:mam_success:notify_deliver_to_offline_user_test{error,{test_case_failed,"Respond size is 4, 1 is expected."}} amp_big_SUITE:mam:mam_success:notify_deliver_to_offline_user_test{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"bOb_notify_deliver_to_offline_user_test_0.278163@localhost/new-session">>,
escalus_tcp,<0.4318.0>,
[{event_manager,<0.3662.0>},
{server,<<"localhost">>},
{username,
<<"bOb_notify_deliver_to_offline_user_test_0.278163">>},
{resource,<<"new-session">>}],
[{event_client,
[{event_manager,<0.3662.0>},
{server,<<"localhost">>},
{username,
<<"bOb_notify_deliver_to_offline_user_test_0.278163">>},
{resource,<<"new-session">>}]},
{resource,<<"new-session">>},
{username,
<<"bOb_notify_deliver_to_offline_user_test_0.278163">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,
<<"bOb_notify_deliver_to_offline_user_test_0.278163">>},
{server,<<"localhost">>},
{password,<<"makrolika">>},
{stream_id,<<"864b3ce4700dfe87">>}]},
5000],
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{mam_helper,wait_archive_respond_v04plus,1,
[{file,"/home/circleci/app/big_tests/tests/mam_helper.erl"},
{line,159}]},
{mam_helper,wa... pgsql_mnesia_23 / pgsql_mnesia / 22cc73c rest_client_SUITE:muc:messages_can_be_paginated_in_room{error,
{{assertion_failed,assert,is_chat_message,
[<<"21d8b493">>],
{xmlel,<<"message">>,
[{<<"to">>,<<"[email protected]">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"302ab7ed">>}]}]},
"<message to='[email protected]' type='chat'><body>302ab7ed</body></message>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{rest_client_SUITE,assert_room_messages,2,
[{file,"/home/circleci/app/big_tests/tests/rest_client_SUITE.erl"},
{line,772}]},
{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,1784}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1293}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1225}]}]}} elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 22cc73c mssql_mnesia_24 / odbc_mssql_mnesia / 22cc73c carboncopy_SUITE:all:unavailable_resources_dont_get_carbons{error,{{assertion_failed,assert_many,false,[is_presence,is_presence],[],[]},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{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,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} carboncopy_SUITE:all:prop_forward_sent_chat_messages{error,
{{assertEqual,
[{module,carboncopy_SUITE},
{line,278},
{expression,
"proper : quickcheck ( proper : conjunction ( [ { PropName , Property } ] ) , [ verbose , long_result , { numtests , 3 } ] )"},
{expected,true},
{value,
[[{forward_sent,
[{3,
<<"This old moon wanes! she lingers my desires">>}]}]]}]},
[{carboncopy_SUITE,run_prop,2,
[{file,"/home/circleci/app/big_tests/tests/carboncopy_SUITE.erl"},
{line,278}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} carboncopy_SUITE:all:prop_normal_routing_to_bare_jid{error,
{{assertEqual,
[{module,carboncopy_SUITE},
{line,278},
{expression,
"proper : quickcheck ( proper : conjunction ( [ { PropName , Property } ] ) , [ verbose , long_result , { numtests , 3 } ] )"},
{expected,true},
{value,
[[{normal_routing,
[{3,<<"Long withering out a young man revenue.">>}]}]]}]},
[{carboncopy_SUITE,run_prop,2,
[{file,"/home/circleci/app/big_tests/tests/carboncopy_SUITE.erl"},
{line,278}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} carboncopy_SUITE:all:unavailable_resources_dont_get_carbons{error,{{assertion_failed,assert_many,false,[is_presence,is_presence],[],[]},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{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,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} mysql_redis_24 / mysql_redis / 22cc73c riak_mnesia_24 / riak_mnesia / 22cc73c pgsql_mnesia_24 / pgsql_mnesia / 22cc73c mod_event_pusher_rabbit_SUITE:group_chat_message_publish:group_chat_message_received_event_properly_formatted{error,
{{assertMatch,
[{module,mod_event_pusher_rabbit_SUITE},
{line,435},
{expression,
"get_decoded_message_from_rabbit ( AliceGroupChatMsgRecvRK )"},
{pattern,
"# { << \"from_user_id\" >> := BobRoomJID , << \"to_user_id\" >> := AliceFullJID , << \"message\" >> := Message }"},
{value,
#{<<"from_user_id">> => <<"[email protected]">>,
<<"message">> => <<>>,
<<"to_user_id">> =>
<<"alice_unnamed_24.733169@localhost/res1">>}}]},
[{mod_event_pusher_rabbit_SUITE,
'-group_chat_message_received_event_properly_formatted/1-fun-1-',3,
[{file,
"/home/circleci/app/big_tests/tests/mod_event_pusher_rabbit_SUITE.erl"},
{line,435}]},
{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,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate the effort towards more performant code, but while I approved the previous version, I cannot approve this one. It sacrifices readability, clarity and simplicity for questionable premature optimizations. Could you just keep it simple?
Some changes, like using the persistent term, are exactly what I was going to do in subsequent PR's, but it can be done here, I have no problem with this. Just please make the parts I commented below more straightforward.
src/backend_module.erl
Outdated
%% Callback implemented by proxy modules. | ||
-callback backend() -> module(). | ||
-compile({inline, [backend_key/2]}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason for such optimizations at this point. They also bring unwanted surprise when debugging.
small_tests_24 / small_tests / cf47c7d small_tests_23 / small_tests / cf47c7d internal_mnesia_24 / internal_mnesia / cf47c7d dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / cf47c7d ldap_mnesia_24 / ldap_mnesia / cf47c7d dynamic_domains_mysql_redis_24 / mysql_redis / cf47c7d ldap_mnesia_23 / ldap_mnesia / cf47c7d dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / cf47c7d rest_client_SUITE:muc:messages_can_be_paginated_in_room{error,
{{assertion_failed,assert,is_chat_message,
[<<"c26498e6">>],
{xmlel,<<"message">>,
[{<<"to">>,<<"[email protected]">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"c8d0aa16">>}]}]},
"<message to='[email protected]' type='chat'><body>c8d0aa16</body></message>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{rest_client_SUITE,assert_room_messages,2,
[{file,"/home/circleci/app/big_tests/tests/rest_client_SUITE.erl"},
{line,772}]},
{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,1784}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1293}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1225}]}]}} dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / cf47c7d pgsql_mnesia_24 / pgsql_mnesia / cf47c7d mysql_redis_24 / mysql_redis / cf47c7d elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / cf47c7d mod_ping_SUITE:server_ping:server_ping_pong{error,{{badmatch,[{[<<"localhost">>,mod_ping,ping_response],
{expected_diff,5},
{before_story,0},
{after_story,4}}]},
[{escalus_mongooseim,post_story_check_metrics,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,74}]},
{escalus_mongooseim,maybe_check_metrics_post_story,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,51}]},
{escalus_story,story,4,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,75}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} mssql_mnesia_24 / odbc_mssql_mnesia / cf47c7d pgsql_mnesia_23 / pgsql_mnesia / cf47c7d riak_mnesia_24 / riak_mnesia / cf47c7d |
There was a problem hiding this 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, just please someone check the specs if they are all correct.
Move new code into mongoose_backend, just to keep it away from the old backend_module Align args correctly
cf47c7d
to
ecf2183
Compare
small_tests_24 / small_tests / ecf2183 internal_mnesia_24 / internal_mnesia / ecf2183 small_tests_23 / small_tests / ecf2183 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / ecf2183 dynamic_domains_mysql_redis_24 / mysql_redis / ecf2183 service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join{error,{test_case_failed,{[<<"example1.com">>],
[<<"example1.com">>,<<"example2.com">>]}}} ldap_mnesia_24 / ldap_mnesia / ecf2183 ldap_mnesia_23 / ldap_mnesia / ecf2183 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / ecf2183 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / ecf2183 pgsql_mnesia_24 / pgsql_mnesia / ecf2183 mod_ping_SUITE:server_ping_kill:server_ping_pong{error,{{badmatch,[{[<<"localhost">>,mod_ping,ping_response],
{expected_diff,5},
{before_story,5},
{after_story,9}}]},
[{escalus_mongooseim,post_story_check_metrics,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,74}]},
{escalus_mongooseim,maybe_check_metrics_post_story,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,51}]},
{escalus_story,story,4,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,75}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / ecf2183 pgsql_mnesia_23 / pgsql_mnesia / ecf2183 mssql_mnesia_24 / odbc_mssql_mnesia / ecf2183 mysql_redis_24 / mysql_redis / ecf2183 riak_mnesia_24 / riak_mnesia / ecf2183 dynamic_domains_mysql_redis_24 / mysql_redis / ecf2183 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks even better 🙂
This PR addresses "MIM-1359 Get rid of dynamically compiled modules - POC"
Proposed changes include: