MIM-1539 Allow global to be passed into mongoose_backend as an arg #3386
Conversation
|
small_tests_24 / small_tests / 6c47fe4 internal_mnesia_24 / internal_mnesia / 6c47fe4 small_tests_23 / small_tests / 6c47fe4 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 6c47fe4 dynamic_domains_mysql_redis_24 / mysql_redis / 6c47fe4 ldap_mnesia_24 / ldap_mnesia / 6c47fe4 ldap_mnesia_23 / ldap_mnesia / 6c47fe4 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">> =>
<<"muc_publish@muc.localhost/bOb_unnamed_92.559415">>,
<<"message">> => <<"Hi there!">>,
<<"to_user_id">> =>
<<"alice_unnamed_92.677872@localhost/res1">>}}]},
[{mod_event_pusher_rabbit_SUITE,
'-group_chat_message_received_event_properly_formatted/1-fun-0-',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,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_pgsql_mnesia_23 / pgsql_mnesia / 6c47fe4 pgsql_mnesia_24 / pgsql_mnesia / 6c47fe4 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 6c47fe4 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 6c47fe4 mssql_mnesia_24 / odbc_mssql_mnesia / 6c47fe4 mysql_redis_24 / mysql_redis / 6c47fe4 pgsql_mnesia_23 / pgsql_mnesia / 6c47fe4 riak_mnesia_24 / riak_mnesia / 6c47fe4 |
Codecov Report
@@ Coverage Diff @@
## without-dynamic-backend-modules #3386 +/- ##
===================================================================
+ Coverage 80.88% 80.89% +0.01%
===================================================================
Files 413 414 +1
Lines 32631 32640 +9
===================================================================
+ Hits 26393 26405 +12
+ Misses 6238 6235 -3
Continue to review full report at Codecov.
|
| @@ -1074,9 +1068,4 @@ get_cached_unique_count() -> | |||
|
|
|||
| -spec sm_backend() -> backend(). | |||
| sm_backend() -> | |||
There was a problem hiding this comment.
With the new changes, is this function even used? If it's not, I don't see a point keeping it, as HostTypes (or, until it's changed, global) should be enough to determine which backend is used.
There was a problem hiding this comment.
it is used in tests
There was a problem hiding this comment.
Honestly I find this a mess, what are we trying to do? Where is the place where we acknowledge this is not host_type ready and it only accepts the global key? I think that that should be well defined somewhere in a unique place. Right now some places in ejabberd_sm are ignoring the HostType vars and passing globaland then again that is done here. Then this is also duplicating knowledge of which one is the backend both in ejabberd_sm and in ejabberd_gen_sm, that should also happen in only one place.
|
To answer @NelsonVides , what we are trying to do, is to allow |
Yeah, I know, I'm aware of all of that excatly, the thing is that introducing Also, |
|
Not renaming HostType argument in mongoose_backend. |
|
small_tests_24 / small_tests / 34fc43b internal_mnesia_24 / internal_mnesia / 34fc43b small_tests_23 / small_tests / 34fc43b dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 34fc43b ldap_mnesia_23 / ldap_mnesia / 34fc43b dynamic_domains_mysql_redis_24 / mysql_redis / 34fc43b dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 34fc43b ldap_mnesia_24 / ldap_mnesia / 34fc43b dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 34fc43b elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 34fc43b pgsql_mnesia_23 / pgsql_mnesia / 34fc43b jingle_SUITE:all:jingle_session_is_established_when_calling_a_number{error,
{{assertion_failed,assert,is_iq_result,
{xmlel,<<"iq">>,
[{<<"from">>,<<"+488790@numbers.localhost">>},
{<<"to">>,
<<"alice_jingle_session_is_established_when_calling_a_number_12.360541@localhost/res1">>},
{<<"id">>,<<"180d19d5-6608-47fb-9a86-fc502e95b78e">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"jingle">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:1">>},
{<<"action">>,<<"session-info">>},
{<<"sid">>,<<"6a28a420-de33-411e-840b-9981c8bea83a">>}],
[{xmlel,<<"ringing">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:apps:rtp:info:1">>}],
[]}]}]},
"<iq from='+488790@numbers.localhost' to='alice_jingle_session_is_established_when_calling_a_number_12.360541@localhost/res1' id='180d19d5-6608-47fb-9a86-fc502e95b78e' type='set'><jingle xmlns='urn:xmpp:jingle:1' action='session-info' sid='6a28a420-de33-411e-840b-9981c8bea83a'><ringing xmlns='urn:xmpp:jingle:apps:rtp:info:1'/></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,
'-jingle_session_is_established_when_calling_a_number/1-fun-0-',1,
[{file,"/home/circleci/app/big_tests/tests/jingle_SUITE.erl"},
{lin...pgsql_mnesia_24 / pgsql_mnesia / 34fc43b mssql_mnesia_24 / odbc_mssql_mnesia / 34fc43b service_domain_db_SUITE:db:db_reinserted_from_one_node_while_service_disabled_on_another{error,
{{badmatch,{ok,<<"dbgroup2">>}},
[{service_domain_db_SUITE,
db_reinserted_from_one_node_while_service_disabled_on_another,1,
[{file,
"/home/circleci/app/big_tests/tests/service_domain_db_SUITE.erl"},
{line,547}]},
{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}]}]}}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">>]}}}mysql_redis_24 / mysql_redis / 34fc43b riak_mnesia_24 / riak_mnesia / 34fc43b |
There was a problem hiding this comment.
Now this seems more organized, and it's clear that this is simply a global module, and if we want to make it per-host-type in the future, all the necessary changes are clear and straightforward (albeit long and mechanical, yeah, big diff, but very predictable and organised diff).
Just have one more comment 👌🏽
NelsonVides
dismissed
their stale review
Not opposed to it anymore, just a minor comment away from approval :)
|
small_tests_24 / small_tests / 9299add internal_mnesia_24 / internal_mnesia / 9299add small_tests_23 / small_tests / 9299add dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 9299add ldap_mnesia_24 / ldap_mnesia / 9299add dynamic_domains_mysql_redis_24 / mysql_redis / 9299add dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 9299add ldap_mnesia_23 / ldap_mnesia / 9299add pgsql_mnesia_24 / pgsql_mnesia / 9299add dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 9299add elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 9299add mysql_redis_24 / mysql_redis / 9299add pgsql_mnesia_23 / pgsql_mnesia / 9299add mssql_mnesia_24 / odbc_mssql_mnesia / 9299add riak_mnesia_24 / riak_mnesia / 9299add |
With the changes done in #3386, this turned out to be really easy. Before that, I tried to make mongoose_rdbms_backend expose its API with HostTypes, but it was a bit messy. I think it's better to do it in a separate PR (after the whole without-dynamic-backend-modules branch is merged), to keep this already big diff smaller and more focused. Other issue is that mongoose_rdbms has many functions exported, which seem to be not used, mainly for escaping different types. This could be revisited, but, in similar fashion, I left this as is for now. What's left to do is remove dynamic mongoose_rdbms_type, but it will be done in a separate PR.
This PR addresses MIM-1539
Proposed changes include: