Cache consistency #3330
Cache consistency #3330
Conversation
|
@chrzaszcz I don't really have a preference on how to properly design the configuration for this, please suggest anything else you'd prefer 🙂 |
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3330 +/- ##
==========================================
- Coverage 80.90% 72.87% -8.04%
==========================================
Files 414 414
Lines 32437 32301 -136
==========================================
- Hits 26244 23540 -2704
- Misses 6193 8761 +2568
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NelsonVides
force-pushed
the
cache_consistency
branch
2 times, most recently
from
fac54ff
to
b57c26e
Compare
This comment has been minimized.
This comment has been minimized.
NelsonVides
force-pushed
the
cache_consistency
branch
from
b57c26e
to
8d89a7b
Compare
This comment has been minimized.
This comment has been minimized.
src/mam/mod_mam_meta.erl
Outdated
| {cache_config, CacheOpts} -> | ||
| Deps1 = case gen_mod:get_opt(cache_module, CacheOpts, internal) of | ||
| internal -> Deps; | ||
| mod_cache_users -> add_dep(mod_cache_users, CacheOpts, Deps) |
There was a problem hiding this comment.
What would happen if mod_cache_users is configured on its own with different options?
There was a problem hiding this comment.
That is a fantastic question, and I can only guess (expect!) that gen_mod_deps is then choosing those configured for the actual module, like, mam won't override what mod_cache_users actually chose. But, I don't actually know, will check.
A similar issue we'd have for mod_blocking against mod_privacy btw, that's why I naively expect that problem to be solve already 😓
There was a problem hiding this comment.
I checked this both in the code and in the live shell and the options are merged with overriding, but it's difficult to predict which one will win - in my case the ones from mod_mam_meta took precedence. It depends on the order of modules, which (as I quickly checked) currently is alphabetical and the last one wins.
when=2021-11-16T08:29:45.513101+00:00 level=warning what=overriding_argument pid=<0.419.0> at=gen_mod_deps:merge_args/3:154 old_value={time_to_live,2} new_value={time_to_live,42} module=mod_cache_users
My recommendation is in this case to completely forbid setting the cache config options in case mod_cache_users is used instead of internal to prevent confusion. This way we would also get rid of the whole extra complexity in mod_mam_meta, which is IMO complex enough.
Anyway, the deps are declared twice: once in mod_mam_cache_user without options and then again in mod_mam_meta with options. Let's just leave the latter and it will be fine.
There was a problem hiding this comment.
If there should be no options here, why not do add_dep(mod_cache_users, Deps)? For me it would be more straightforward.
This comment has been minimized.
This comment has been minimized.
NelsonVides
force-pushed
the
cache_consistency
branch
from
1665483
to
6b6900d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/mam/mod_mam_meta.erl
Outdated
| {cache_config, CacheOpts} -> | ||
| Deps1 = case gen_mod:get_opt(cache_module, CacheOpts, internal) of | ||
| internal -> Deps; | ||
| mod_cache_users -> add_dep(mod_cache_users, CacheOpts, Deps) |
There was a problem hiding this comment.
I checked this both in the code and in the live shell and the options are merged with overriding, but it's difficult to predict which one will win - in my case the ones from mod_mam_meta took precedence. It depends on the order of modules, which (as I quickly checked) currently is alphabetical and the last one wins.
when=2021-11-16T08:29:45.513101+00:00 level=warning what=overriding_argument pid=<0.419.0> at=gen_mod_deps:merge_args/3:154 old_value={time_to_live,2} new_value={time_to_live,42} module=mod_cache_users
My recommendation is in this case to completely forbid setting the cache config options in case mod_cache_users is used instead of internal to prevent confusion. This way we would also get rid of the whole extra complexity in mod_mam_meta, which is IMO complex enough.
Anyway, the deps are declared twice: once in mod_mam_cache_user without options and then again in mod_mam_meta with options. Let's just leave the latter and it will be fine.
This comment has been minimized.
This comment has been minimized.
src/mam/mod_mam_meta.erl
Outdated
| {cache_config, CacheOpts} -> | ||
| Deps1 = case gen_mod:get_opt(cache_module, CacheOpts, internal) of | ||
| internal -> Deps; | ||
| mod_cache_users -> add_dep(mod_cache_users, CacheOpts, Deps) |
There was a problem hiding this comment.
If there should be no options here, why not do add_dep(mod_cache_users, Deps)? For me it would be more straightforward.
| %% path: (host_config[].)modules.*.cache | ||
| config_spec() -> | ||
| #section{ | ||
| items = #{<<"module">> => #option{type = atom, validate = {enum, [internal, mod_cache_users]}}, |
There was a problem hiding this comment.
It is not a blocker, but I would omit this option in mod_cache_users as it does not make sense there, just for better clarity - it confused me a bit when reading the code. Config spec can be used as a reference and I think that unused options should not be there.
test/config_parser_SUITE.erl
Outdated
| @@ -2164,11 +2164,11 @@ test_mod_mam_meta(T, M) -> | |||
| ?errf(T(#{<<"simple">> => <<"yes">>})), | |||
| ?errf(T(#{<<"extra_fin_element">> => <<"bad_module">>})), | |||
| ?errf(T(#{<<"extra_lookup_params">> => <<"bad_module">>})), | |||
| ?errf(T(#{<<"cache_config">> => #{<<"cache_module">> => <<"mod_wrong_cache">>}})), | |||
There was a problem hiding this comment.
I think we could add a small test here with module set to internal and some options to make sure it produces an error.
The modules shouldn't introduce dependencies between them, so instead the mam module referring to mod_cache_users, we create a bridge module called mongoose_user_cache, that abstracts away how users use the segmented_cache library, and how modules initialise their configuration and keys. Also, we disable configuring any parameter when the module is set to any external value.
NelsonVides
force-pushed
the
cache_consistency
branch
from
66e1c41
to
64a6275
Compare
|
small_tests_24 / small_tests / 64a6275 internal_mnesia_24 / internal_mnesia / 64a6275 small_tests_23 / small_tests / 64a6275 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 64a6275 dynamic_domains_mysql_redis_24 / mysql_redis / 64a6275 mam_SUITE:rdbms_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}mongooseimctl_SUITE:stats:stats_global{error,{{badmatch,{"5\n",0}},
[{mongooseimctl_SUITE,'-stats_global/1-fun-0-',3,
[{file,"/home/circleci/app/big_tests/tests/mongooseimctl_SUITE.erl"},
{line,1057}]},
{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 / 64a6275 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 64a6275 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 64a6275 ldap_mnesia_23 / ldap_mnesia / 64a6275 pgsql_mnesia_24 / pgsql_mnesia / 64a6275 mongooseimctl_SUITE:stats:stats_global{error,{{badmatch,{"5\n",0}},
[{mongooseimctl_SUITE,'-stats_global/1-fun-0-',3,
[{file,"/home/circleci/app/big_tests/tests/mongooseimctl_SUITE.erl"},
{line,1057}]},
{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}]}]}}elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 64a6275 pgsql_mnesia_23 / pgsql_mnesia / 64a6275 mysql_redis_24 / mysql_redis / 64a6275 mam_SUITE:rdbms_prefs_cases:messages_filtered_when_prefs_default_policy_is_never{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_stop_c2s{error,{thrown,{timeout,msg}}}mssql_mnesia_24 / odbc_mssql_mnesia / 64a6275 pep_SUITE:pep_tests:delayed_receive{error,{{badmatch,[]},
[{pep_SUITE,'-delayed_receive/1-fun-0-',3,
[{file,"/home/circleci/app/big_tests/tests/pep_SUITE.erl"},
{line,276}]},
{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}]}]}}pep_SUITE:pep_tests:send_caps_after_login_test{error,{{assertion_failed,assert_many,false,
[is_roster_set,is_presence,is_presence],
[{xmlel,<<"presence">>,
[{<<"from">>,
<<"bOb_send_caps_after_login_test_71.502629@localhost/res1">>},
{<<"to">>,
<<"alice_send_caps_after_login_test_71.502629@localhost/res1">>},
{<<"xml:lang">>,<<"en">>}],
[]}],
" <presence from='bOb_send_caps_after_login_test_71.502629@localhost/res1' to='alice_send_caps_after_login_test_71.502629@localhost/res1' xml:lang='en'/>"},
[{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,'-make_all_clients_friends/1-fun-0-',2,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,114}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,distinct_pairs,2,
[{file,"...riak_mnesia_24 / riak_mnesia / 64a6275 dynamic_domains_mysql_redis_24 / mysql_redis / 64a6275 mam_SUITE:rdbms_cache_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}pgsql_mnesia_24 / pgsql_mnesia / 64a6275 service_domain_db_SUITE:db:db_keeps_syncing_after_cluster_join{error,{test_case_failed,{[<<"example1.com">>],
[<<"example1.com">>,<<"example2.com">>]}}}service_domain_db_SUITE:db:rest_with_auth:rest_delete_domain_cleans_data_from_mam{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"bob_rest_delete_domain_cleans_data_from_mam_82.622188@example.org/res1">>,
escalus_tcp,<0.19015.2>,
[{event_manager,<0.19009.2>},
{server,<<"example.org">>},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_82.622188">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.19009.2>},
{server,<<"example.org">>},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_82.622188">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_82.622188">>},
{server,<<"example.org">>},
{host,<<"localhost">>},
{port,5232},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_82.622188">>},
{server,<<"example.org">>},
{host,<<"localhost">>},
{password,<<"makota3">>},
{port,5232},
{stream_id,<<"0cffcc4a0ff9370c">>}]},
5000],
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{service_domain_db_SUITE,
'-rest_delete_domain_cleans_data_from_mam/1-fun-0-',5...service_domain_db_SUITE:db:rest_without_auth:rest_delete_domain_cleans_data_from_mam{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"bob_rest_delete_domain_cleans_data_from_mam_90.698970@example.org/res1">>,
escalus_tcp,<0.19655.2>,
[{event_manager,<0.19649.2>},
{server,<<"example.org">>},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_90.698970">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.19649.2>},
{server,<<"example.org">>},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_90.698970">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_90.698970">>},
{server,<<"example.org">>},
{host,<<"localhost">>},
{port,5232},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,
<<"bob_rest_delete_domain_cleans_data_from_mam_90.698970">>},
{server,<<"example.org">>},
{host,<<"localhost">>},
{password,<<"makota3">>},
{port,5232},
{stream_id,<<"6f5ffabfe67647ed">>}]},
5000],
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{service_domain_db_SUITE,
'-rest_delete_domain_cleans_data_from_mam/1-fun-0-',5...pgsql_mnesia_24 / pgsql_mnesia / 64a6275 |
|
small_tests_24 / small_tests / 55a6deb internal_mnesia_24 / internal_mnesia / 55a6deb small_tests_23 / small_tests / 55a6deb dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 55a6deb dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 55a6deb ldap_mnesia_24 / ldap_mnesia / 55a6deb sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_send_message{error,
{{assertion_failed,assert_many,true,
[#Fun<sm_SUITE.22.149602>,#Fun<sm_SUITE.22.149602>,
#Fun<sm_SUITE.22.149602>],
[{xmlel,<<"presence">>,
[{<<"from">>,
<<"alicE_resume_session_state_send_message_87.478353@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alice_resume_session_state_send_message_87.478353@localhost/escalus-default-resource">>},
{<<"xml:lang">>,<<"en">>}],
[]},
{xmlel,<<"message">>,
[{<<"from">>,
<<"bOb_resume_session_state_send_message_87.430965@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alicE_resume_session_state_send_message_87.478353@localhost">>},
{<<"xml:lang">>,<<"en">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-1">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>},
{<<"stamp">>,<<"2021-11-17T17:44:47.547168Z">>},
{<<"from">>,<<"localhost">>}],
[{xmlcdata,<<"SM Storage">>}]}]},
{xmlel,<<"message">>,
[{<<"from">>,
<<"bOb_resume_session_state_send_message_87.430965@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alicE_resume_session_state_send_message_87.478353@localhost">>},
{<<"xml:lang">>,<<"en">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-2">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay"...dynamic_domains_mysql_redis_24 / mysql_redis / 55a6deb mam_SUITE:rdbms_async_pool_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}}dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 55a6deb carboncopy_SUITE:all:prop_forward_received_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_received,
[{5,<<"Now, fair Hippolyta, our nuptial hour">>}]}]]}]},
[{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,
[{5,<<"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}]}]}}ldap_mnesia_23 / ldap_mnesia / 55a6deb elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 55a6deb pgsql_mnesia_24 / pgsql_mnesia / 55a6deb mssql_mnesia_24 / odbc_mssql_mnesia / 55a6deb mysql_redis_24 / mysql_redis / 55a6deb pgsql_mnesia_23 / pgsql_mnesia / 55a6deb riak_mnesia_24 / riak_mnesia / 55a6deb sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_send_message{error,
{{assertion_failed,assert_many,true,
[#Fun<sm_SUITE.22.149602>,#Fun<sm_SUITE.22.149602>,
#Fun<sm_SUITE.22.149602>],
[{xmlel,<<"presence">>,
[{<<"from">>,
<<"alicE_resume_session_state_send_message_18.170186@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alice_resume_session_state_send_message_18.170186@localhost/escalus-default-resource">>},
{<<"xml:lang">>,<<"en">>}],
[]},
{xmlel,<<"message">>,
[{<<"from">>,
<<"bOb_resume_session_state_send_message_18.46528@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alicE_resume_session_state_send_message_18.170186@localhost">>},
{<<"xml:lang">>,<<"en">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-1">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>},
{<<"stamp">>,<<"2021-11-17T17:53:38.401681Z">>},
{<<"from">>,<<"localhost">>}],
[{xmlcdata,<<"SM Storage">>}]}]},
{xmlel,<<"message">>,
[{<<"from">>,
<<"bOb_resume_session_state_send_message_18.46528@localhost/escalus-default-resource">>},
{<<"to">>,
<<"alicE_resume_session_state_send_message_18.170186@localhost">>},
{<<"xml:lang">>,<<"en">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-2">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>... |
Unify caches, use segmented_cache as a cache backend and have all caches use it.
mod_cache_usersowns its own cache instance, and mam, which had two caches, one for users and one for rooms, was first unified to have a single cache, and then introduced the possibility to reusemod_cache_userscache entity.