-
Notifications
You must be signed in to change notification settings - Fork 429
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
Add retry logic into mongooseim task aggregator #3969
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3969 +/- ##
=======================================
Coverage 83.54% 83.54%
=======================================
Files 538 538
Lines 34007 34022 +15
=======================================
+ Hits 28412 28425 +13
- Misses 5595 5597 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
In general it looks good 👍 I left a few minor comments to consider. Also, CI won't allow to merge because of the coverage decrease. Please look into it.
%% Reset number of retries | ||
maybe_request_next2(State#state{retries_left = Total}). | ||
|
||
maybe_request_next2(#state{flush_elems = Acc, flush_queue = Queue} = State) -> |
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.
Maybe instead of creating a function with suffix 2 we could go with something like this:
maybe_request_next(#state{flush_elems = Acc, flush_queue = Queue, total_retries = Total, retries_left = Total} = State) ->
case queue:out(Queue) of
{{value, Key}, NewQueue} ->
{Value, NewAcc} = maps:take(Key, Acc),
NewState1 = State#state{flush_elems = NewAcc, flush_queue = NewQueue},
case make_async_request(Value, NewState1) of
NewState2 = #state{async_request = no_request_pending} ->
maybe_request_next(NewState2);
NewState2 ->
NewState2
end;
{empty, _} ->
State#state{async_request = no_request_pending}
end;
maybe_request_next(#state{total_retries = Total} = State) ->
%% Reset number of retries
maybe_request_next(State#state{retries_left = Total}).
What do you think?
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 can use State1 = State#state{retries_left = Total}
.
Don't like recursion here, makes code a bit harder.
yeah, coverage decrease is annoying, because we didn't had a test for |
small_tests_24 / small_tests / 08a9a33 small_tests_25 / small_tests / 08a9a33 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 08a9a33 ldap_mnesia_25 / ldap_mnesia / 08a9a33 dynamic_domains_mysql_redis_25 / mysql_redis / 08a9a33 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 08a9a33 pgsql_mnesia_24 / pgsql_mnesia / 08a9a33 push_integration_SUITE:pubsub_less:pm_notifications_with_inbox:inbox_msg_reset_unread_count_apns{error,
{{assertion_failed,assert_many,false,
[is_presence,is_message,is_message],
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_inbox_msg_reset_unread_count_apns_2894@localhost/res1">>},
{<<"to">>,
<<"bob_inbox_msg_reset_unread_count_apns_2894@localhost">>},
{<<"type">>,<<"chat">>},
{<<"id">>,<<"4b464c6e210fee3d8db78336c0e6c20e">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"FIRST MESSAGE">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>},
{<<"stamp">>,<<"2023-02-27T17:34:36.567605Z">>},
{<<"from">>,<<"localhost">>}],
[{xmlcdata,<<"Offline Storage">>}]}]},
{xmlel,<<"presence">>,
[{<<"from">>,
<<"bob_inbox_msg_reset_unread_count_apns_2894@localhost/res1">>},
{<<"to">>,
<<"bob_inbox_msg_reset_unread_count_apns_2894@localhost/res1">>}],
[]}],
" <message from='alice_inbox_msg_reset_unread_count_apns_2894@localhost/res1' to='bob_inbox_msg_reset_unread_count_apns_2894@localhost' type='chat' id='4b464c6e210fee3d8db78336c0e6c20e'><body>FIRST MESSAGE</body><delay xmlns='urn:xmpp:delay' stamp='2023-02-27T17:34:36.567605Z' from='localhost'>Offline Storage</delay></message> <presence from='bob_inbox_msg_reset_unread_count_apns_2894@localhost/res1' to='bob_inbox_msg_reset_unread_count_apns_2894@localhost/res1'/>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/project/big_tes... dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 08a9a33 internal_mnesia_25 / internal_mnesia / 08a9a33 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 08a9a33 pgsql_mnesia_25 / pgsql_mnesia / 08a9a33 riak_mnesia_24 / riak_mnesia / 08a9a33 mysql_redis_25 / mysql_redis / 08a9a33 mssql_mnesia_25 / odbc_mssql_mnesia / 08a9a33 pgsql_mnesia_24 / pgsql_mnesia / 08a9a33 |
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.
LGTM 👍
small_tests_24 / small_tests / 4139c9d small_tests_25 / small_tests / 4139c9d ldap_mnesia_24 / ldap_mnesia / 4139c9d dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 4139c9d bosh_SUITE:essential_https:accept_higher_hold_value{error,
{{assertEqual,
[{module,bosh_SUITE},
{line,251},
{expression,"get_bosh_sessions ( )"},
{expected,[]},
{value,
[{bosh_session,<<"3e4dc42eeaf8b1c22b14c2daa68f9378b48535d2">>,
<8839.10055.0>}]}]},
[{bosh_SUITE,accept_higher_hold_value,1,
[{file,"/home/circleci/project/big_tests/tests/bosh_SUITE.erl"},
{line,251}]},
{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_25 / ldap_mnesia / 4139c9d dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 4139c9d dynamic_domains_mysql_redis_25 / mysql_redis / 4139c9d dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 4139c9d inbox_extensions_SUITE:regular:one_to_one:pagination:pagination_overrides_form{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"bob_pagination_overrides_form_1499@domain.example.com/res1">>,
escalus_tcp,<0.13215.1>,
[{event_manager,<0.13180.1>},
{server,<<"domain.example.com">>},
{username,<<"bOb_pagination_overrides_form_1499">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.13180.1>},
{server,<<"domain.example.com">>},
{username,<<"bOb_pagination_overrides_form_1499">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"bob_pagination_overrides_form_1499">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"bOb_pagination_overrides_form_1499">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"makrolika">>},
{stream_id,<<"8e8d9083f0f69555">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{inbox_helper,'-given_conversations_between/2-fun-1-',4,
[{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
{line,543}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1350}]},
{inbox_e... inbox_extensions_SUITE:regular:one_to_one:pagination:max_queries_can_fetch_ahead{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"bob_max_queries_can_fetch_ahead_1501@domain.example.com/res1">>,
escalus_tcp,<0.13224.1>,
[{event_manager,<0.13194.1>},
{server,<<"domain.example.com">>},
{username,<<"bOb_max_queries_can_fetch_ahead_1501">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.13194.1>},
{server,<<"domain.example.com">>},
{username,<<"bOb_max_queries_can_fetch_ahead_1501">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"bob_max_queries_can_fetch_ahead_1501">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"bOb_max_queries_can_fetch_ahead_1501">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"makrolika">>},
{stream_id,<<"009353d8bb7669e8">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{inbox_helper,'-given_conversations_between/2-fun-1-',4,
[{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
{line,543}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1350}]},
... inbox_extensions_SUITE:regular:one_to_one:pagination:max_queries_can_be_limited{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"kate_max_queries_can_be_limited_1503@domain.example.com/res1">>,
escalus_tcp,<0.13227.1>,
[{event_manager,<0.13208.1>},
{server,<<"domain.example.com">>},
{username,<<"kate_max_queries_can_be_limited_1503">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.13208.1>},
{server,<<"domain.example.com">>},
{username,<<"kate_max_queries_can_be_limited_1503">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"kate_max_queries_can_be_limited_1503">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"kate_max_queries_can_be_limited_1503">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"makrowe;p">>},
{stream_id,<<"02af5f0c335c4116">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{inbox_helper,'-given_conversations_between/2-fun-1-',4,
[{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
{line,543}]},
{lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
... inbox_extensions_SUITE:regular:one_to_one:pagination:can_paginate_forwards{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"kate_can_paginate_forwards_1498@domain.example.com/res1">>,
escalus_tcp,<0.13225.1>,
[{event_manager,<0.13173.1>},
{server,<<"domain.example.com">>},
{username,<<"kate_can_paginate_forwards_1498">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.13173.1>},
{server,<<"domain.example.com">>},
{username,<<"kate_can_paginate_forwards_1498">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"kate_can_paginate_forwards_1498">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"kate_can_paginate_forwards_1498">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"makrowe;p">>},
{stream_id,<<"5581033d654cd06d">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{inbox_helper,'-given_conversations_between/2-fun-1-',4,
[{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
{line,543}]},
{lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
{inbox_extensions_SUI... inbox_extensions_SUITE:regular:one_to_one:pagination:can_paginate_backwards{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"kate_can_paginate_backwards_1500@domain.example.com/res1">>,
escalus_tcp,<0.13228.1>,
[{event_manager,<0.13187.1>},
{server,<<"domain.example.com">>},
{username,<<"kate_can_paginate_backwards_1500">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.13187.1>},
{server,<<"domain.example.com">>},
{username,<<"kate_can_paginate_backwards_1500">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"kate_can_paginate_backwards_1500">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"kate_can_paginate_backwards_1500">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"makrowe;p">>},
{stream_id,<<"8b317060f6dcd3a9">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{inbox_helper,'-given_conversations_between/2-fun-1-',4,
[{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
{line,543}]},
{lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
{inbox_extension... internal_mnesia_25 / internal_mnesia / 4139c9d pgsql_mnesia_24 / pgsql_mnesia / 4139c9d riak_mnesia_24 / riak_mnesia / 4139c9d pubsub_SUITE:dag+basic:publish_only_retract_items_scope_test{error,{{badmatch,false},
[{pubsub_tools,check_response,2,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,491}]},
{pubsub_tools,receive_response,3,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,481}]},
{pubsub_tools,receive_and_check_response,4,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,471}]},
{pubsub_SUITE,'-publish_only_retract_items_scope_test/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/tests/pubsub_SUITE.erl"},
{line,723}]},
{escalus_story,story,4,
[{file,"/home/circleci/project/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}]}]}} pgsql_mnesia_25 / pgsql_mnesia / 4139c9d push_integration_SUITE:pubsub_less:pm_notifications_with_inbox:inbox_msg_reset_unread_count_fcm{error,
{{assertion_failed,assert_many,false,
[is_presence,is_message,is_message],
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_inbox_msg_reset_unread_count_fcm_2982@localhost/res1">>},
{<<"to">>,
<<"bob_inbox_msg_reset_unread_count_fcm_2982@localhost">>},
{<<"type">>,<<"chat">>},
{<<"id">>,<<"677b4cfe3ba72d1f63f0047599304f5d">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"FIRST MESSAGE">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>},
{<<"stamp">>,<<"2023-02-28T09:38:53.946378Z">>},
{<<"from">>,<<"localhost">>}],
[{xmlcdata,<<"Offline Storage">>}]}]},
{xmlel,<<"presence">>,
[{<<"from">>,
<<"bob_inbox_msg_reset_unread_count_fcm_2982@localhost/res1">>},
{<<"to">>,
<<"bob_inbox_msg_reset_unread_count_fcm_2982@localhost/res1">>}],
[]}],
" <message from='alice_inbox_msg_reset_unread_count_fcm_2982@localhost/res1' to='bob_inbox_msg_reset_unread_count_fcm_2982@localhost' type='chat' id='677b4cfe3ba72d1f63f0047599304f5d'><body>FIRST MESSAGE</body><delay xmlns='urn:xmpp:delay' stamp='2023-02-28T09:38:53.946378Z' from='localhost'>Offline Storage</delay></message> <presence from='bob_inbox_msg_reset_unread_count_fcm_2982@localhost/res1' to='bob_inbox_msg_reset_unread_count_fcm_2982@localhost/res1'/>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/project/big_tests/_buil... mysql_redis_25 / mysql_redis / 4139c9d mssql_mnesia_25 / odbc_mssql_mnesia / 4139c9d dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 4139c9d dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 4139c9d graphql_server_SUITE:admin_http:clustering_http_tests:remove_node_test{error,{{badmatch,{error,econnrefused}},
[{rest_helper,fusco_request,7,
[{file,"/home/circleci/project/big_tests/tests/rest_helper.erl"},
{line,185}]},
{rest_helper,make_request,1,
[{file,"/home/circleci/project/big_tests/tests/rest_helper.erl"},
{line,114}]},
{rest_helper,make_request,1,
[{file,"/home/circleci/project/big_tests/tests/rest_helper.erl"},
{line,121}]},
{graphql_server_SUITE,remove_node_test,1,
[{file,"/home/circleci/project/big_tests/tests/graphql_server_SUITE.erl"},
{line,210}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1291}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1223}]}]}} graphql_server_SUITE:admin_http:clustering_http_tests:stop_node_test{error,{{badmatch,{error,econnrefused}},
[{rest_helper,fusco_request,7,
[{file,"/home/circleci/project/big_tests/tests/rest_helper.erl"},
{line,185}]},
{rest_helper,make_request,1,
[{file,"/home/circleci/project/big_tests/tests/rest_helper.erl"},
{line,114}]},
{rest_helper,make_request,1,
[{file,"/home/circleci/project/big_tests/tests/rest_helper.erl"},
{line,121}]},
{graphql_server_SUITE,stop_node_test,1,
[{file,"/home/circleci/project/big_tests/tests/graphql_server_SUITE.erl"},
{line,215}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1291}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1223}]}]}} muc_SUITE:register:user_submits_registration_form{error,
{{assertion_failed,assert,is_iq_result,
[{xmlel,<<"iq">>,
[{<<"type">>,<<"set">>},
{<<"id">>,<<"8590eb0afcaa050f5f202105f4d2a0bb">>},
{<<"to">>,<<"groupchats.domain.example.com">>}],
[{xmlel,<<"query">>,
[{<<"xmlns">>,<<"jabber:iq:register">>}],
[{xmlel,<<"x">>,
[{<<"xmlns">>,<<"jabber:x:data">>},
{<<"type">>,<<"submit">>}],
[{xmlel,<<"field">>,
[{<<"type">>,<<"hidden">>},
{<<"var">>,<<"FORM_TYPE">>}],
[{xmlel,<<"value">>,[],
[{xmlcdata,<<"jabber:iq:register">>}]}]},
{xmlel,<<"field">>,
[{<<"type">>,<<"text-single">>},
{<<"var">>,<<"nick">>}],
[{xmlel,<<"value">>,[],
[{xmlcdata,
<<"thirdwitchroom-92ad6cb0e2">>}]}]}]}]}]}],
{xmlel,<<"iq">>,
[{<<"from">>,<<"groupchats.domain.example.com">>},
{<<"to">>,
<<"alice_user_submits_registration_form_2462@domain.example.com/res1">>},
{<<"type">>,<<"error">>},
{<<"id">>,<<"8590eb0afcaa050f5f202105f4d2a0bb">>}],
[{xmlel,<<"query">>,
[{<<"xmlns">>,<<"jabber:iq:register">>}],
[{xmlel,<<"x">>,
[{<<"xmlns">>,<<"jabber:x:data">>},
{<<"type">>,<<"submit">>}],
[{xmlel,<<"field">>,
[{<<"type">>,<<"hidden">>},
{<<"var">>,<<"FORM_TYPE">>}],
... pgsql_mnesia_25 / pgsql_mnesia / 4139c9d dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 4139c9d |
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.
Ok, seems good, convoluted usual retry logic, I'm fine with it. Just some wrong indentation going around 😇
ignore -> | ||
{noreply, State}; | ||
next -> | ||
{noreply, maybe_request_next(State)}; | ||
retry -> | ||
{noreply, maybe_request_retry(ReqTask, State)} |
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.
Indentation is off here
small_tests_24 / small_tests / 110d8f9 small_tests_25 / small_tests / 110d8f9 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 110d8f9 ldap_mnesia_24 / ldap_mnesia / 110d8f9 ldap_mnesia_25 / ldap_mnesia / 110d8f9 dynamic_domains_mysql_redis_25 / mysql_redis / 110d8f9 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 110d8f9 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 110d8f9 pgsql_mnesia_24 / pgsql_mnesia / 110d8f9 internal_mnesia_25 / internal_mnesia / 110d8f9 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 110d8f9 mysql_redis_25 / mysql_redis / 110d8f9 riak_mnesia_24 / riak_mnesia / 110d8f9 pgsql_mnesia_25 / pgsql_mnesia / 110d8f9 mssql_mnesia_25 / odbc_mssql_mnesia / 110d8f9 |
This PR addresses "Failing inbox tests on mssql". A part of MIM-1823 ticket.
Proposed changes include: