-
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
CETS backend for mod_jingle_sip #4076
Conversation
elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 1609279 small_tests_24 / small_tests / 1609279 small_tests_25_arm64 / small_tests / 1609279 small_tests_25 / small_tests / 1609279 ldap_mnesia_24 / ldap_mnesia / 1609279 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 1609279 ldap_mnesia_25 / ldap_mnesia / 1609279 dynamic_domains_mysql_redis_25 / mysql_redis / 1609279 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 1609279 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 1609279 pgsql_mnesia_24 / pgsql_mnesia / 1609279 internal_mnesia_25 / internal_mnesia / 1609279 pgsql_mnesia_25 / pgsql_mnesia / 1609279 pgsql_cets_25 / pgsql_cets / 1609279 mysql_redis_25 / mysql_redis / 1609279 mssql_mnesia_25 / odbc_mssql_mnesia / 1609279 mam_SUITE:rdbms_async_pool_muc_all:muc06:muc_message_with_stanzaid{failed,
{mam_SUITE,end_per_testcase,
{'EXIT',
{{room_archive_size,0,[{times,200,1}],ok},
[{mongoose_helper,do_wait_until,2,
[{file,
"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
{line,358}]},
{mam_helper,wait_for_room_archive_size,3,
[{file,
"/home/circleci/project/big_tests/tests/mam_helper.erl"},
{line,789}]},
{mam_helper,clean_room_archive,1,
[{file,
"/home/circleci/project/big_tests/tests/mam_helper.erl"},
{line,733}]},
{mam_helper,destroy_room,1,
[{file,
"/home/circleci/project/big_tests/tests/mam_helper.erl"},
{line,726}]},
{mam_SUITE,end_per_testcase,2,
[{file,
"/home/circleci/project/big_tests/tests/mam_SUITE.erl"},
{line,954}]},
{test_server,do_end_per_testcase,4,
[{file,"test_server.erl"},{line,1626}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1334}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1223}]}]}}}} |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feature/cets #4076 +/- ##
================================================
+ Coverage 83.65% 83.99% +0.34%
================================================
Files 546 549 +3
Lines 33519 33544 +25
================================================
+ Hits 28040 28176 +136
+ Misses 5479 5368 -111
☔ View full report in Codecov by Sentry. |
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.
Code looks good, but I see a lot of literal code duplication between mod_jingle_sip_mnesia
and mod_jingle_sip_cets
- records, types, entire functions etc. This would make maintenance more difficult. Wouldn't it be better to extract common code to another module? The common functions could even be left in mod_jingle_sip_backend
or a helper module could be created. Record definitions could be placed in an included header file.
|
||
-spec init(mongooseim:host_type(), gen_mod:module_opts()) -> ok. | ||
init(_Host, _Opts) -> | ||
cets:start(?TABLE, #{keypos => 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.
cets:start(?TABLE, #{keypos => 2}), | |
cets:start(?TABLE, #{keypos => #jingle_sip_session.sid}), |
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, I've moved record definition into a header file.
CETS backend is written in a way, it does not know format of the record now.
So, we just use the keypos=2 (i.e. keypos for generic records).
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 just prefer to reference a particular field, so I don't have to check the record definition to see which field it is. If someone moves sid or adds a field before it (I don't know why, but if), then my version would remain valid. Plus, it would be self-descriptive without the need for a comment.
Add only basic functions to mod_jingle_sip_backend Define record type for jingle_sip_session
Made a separation of mod_jingle_sip_backend and mod_jingle_sip_sessions. |
elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / eac044b small_tests_24 / small_tests / eac044b small_tests_25_arm64 / small_tests / eac044b ldap_mnesia_24 / ldap_mnesia / eac044b dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / eac044b ldap_mnesia_25 / ldap_mnesia / eac044b mod_global_distrib_SUITE:hosts_refresher:test_host_refreshing{error,
{{trees_for_connections_present,true,[{times,50,false}],ok},
[{mongoose_helper,do_wait_until,2,
[{file,"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
{line,358}]},
{mod_global_distrib_SUITE,test_host_refreshing,1,
[{file,
"/home/circleci/project/big_tests/tests/mod_global_distrib_SUITE.erl"},
{line,384}]},
{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}]}]}} dynamic_domains_mysql_redis_25 / mysql_redis / eac044b dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / eac044b dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / eac044b pgsql_mnesia_24 / pgsql_mnesia / eac044b 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_cets_25 / pgsql_cets / eac044b internal_mnesia_25 / internal_mnesia / eac044b pgsql_mnesia_25 / pgsql_mnesia / eac044b mod_global_distrib_SUITE:hosts_refresher:test_host_refreshing{error,
{{trees_for_connections_present,true,[{times,50,false}],ok},
[{mongoose_helper,do_wait_until,2,
[{file,"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
{line,358}]},
{mod_global_distrib_SUITE,test_host_refreshing,1,
[{file,
"/home/circleci/project/big_tests/tests/mod_global_distrib_SUITE.erl"},
{line,384}]},
{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}]}]}} mysql_redis_25 / mysql_redis / eac044b mssql_mnesia_25 / odbc_mssql_mnesia / eac044b ldap_mnesia_25 / ldap_mnesia / eac044b pgsql_mnesia_25 / pgsql_mnesia / eac044b pgsql_mnesia_24 / pgsql_mnesia / eac044b mod_global_distrib_SUITE:hosts_refresher:test_host_refreshing{error,
{{trees_for_connections_present,true,[{times,50,false}],ok},
[{mongoose_helper,do_wait_until,2,
[{file,"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
{line,358}]},
{mod_global_distrib_SUITE,test_host_refreshing,1,
[{file,
"/home/circleci/project/big_tests/tests/mod_global_distrib_SUITE.erl"},
{line,384}]},
{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 / eac044b pgsql_mnesia_24 / pgsql_mnesia / eac044b pgsql_mnesia_25 / pgsql_mnesia / eac044b |
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 👍
This PR addresses "MIM-1934 CETS backend for mod_jingle_sip_backend"
Proposed changes include: