-
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
Raise an error if mnesia:create_table/2 fails #4138
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4138 +/- ##
==========================================
- Coverage 84.07% 84.05% -0.02%
==========================================
Files 559 560 +1
Lines 34038 34015 -23
==========================================
- Hits 28617 28592 -25
- Misses 5421 5423 +2
☔ View full report in Codecov by Sentry. |
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.
It looks good, I like the common code being extracted to a separate module.
A few minor comments from me.
@@ -81,7 +81,6 @@ | |||
%% ------------------------ Backend start/stop ------------------------ | |||
|
|||
init(HostType, Opts) -> | |||
mod_pubsub_db_mnesia:init(HostType, Opts), |
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.
Why is it safe to delete this?
Any idea why was the mnesia backend even started here?
Note that mod_pubsub_db_mnesia:stop/2
is still called in line 254.
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.
It passed tests, so haven't investigated too deep. Not sure why it is there in the first place :)
Maybe there is some shared logic used by both backends. I thought there was at some point.
Could check again.
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.
At least that call would do nothing, if someone tries to use mod_pubsub without mnesia configured (i.e. all errors are just ignored inside).
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.
it is safe for start. I think, it was used when someone was adding rdbms backend, but was not cleaned up after rdbms backend was finished.
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 would take a look inside to ensure that the RDBMS backend is not reusing any logic of the mnesia one...
875bc33
to
d952abe
Compare
This comment was marked as outdated.
This comment was marked as outdated.
It has mongoose_mnesia:create_table/2 function, which checks that mnesia table is created correctly.
Remove mod_pubsub_db_mnesia:stop from mod_pubsub_db_rdbms
d952abe
to
40e3f51
Compare
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 40e3f51 small_tests_25 / small_tests / 40e3f51 small_tests_26_arm64 / small_tests / 40e3f51 small_tests_26 / small_tests / 40e3f51 ldap_mnesia_25 / ldap_mnesia / 40e3f51 dynamic_domains_SUITE:with_mod_dynamic_domains_test:packet_handling_for_subdomain{error,
{{badrpc,
{'EXIT',
{timeout,
[{meck_proc,wait,6,
[{file,
"/home/circleci/project/_build/default/lib/meck/src/meck_proc.erl"},
{line,171}]},
{meck,wait,5,[]}]}}},
[{distributed_helper,rpc,
[#{node => mongooseim@localhost},
meck,wait,
[3,mod_dynamic_domains_test,process_packet,5,500]],
[{file,
"/home/circleci/project/big_tests/tests/distributed_helper.erl"},
{line,140}]},
{dynamic_domains_SUITE,'-packet_handling_for_subdomain/1-fun-3-',1,
[{file,
"/home/circleci/project/big_tests/tests/dynamic_domains_SUITE.erl"},
{line,113}]},
{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,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_pgsql_mnesia_25 / pgsql_mnesia / 40e3f51 ldap_mnesia_26 / ldap_mnesia / 40e3f51 dynamic_domains_mysql_redis_26 / mysql_redis / 40e3f51 dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 40e3f51 internal_mnesia_26 / internal_mnesia / 40e3f51 pgsql_mnesia_25 / pgsql_mnesia / 40e3f51 pgsql_cets_26 / pgsql_cets / 40e3f51 mysql_redis_26 / mysql_redis / 40e3f51 dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 40e3f51 pgsql_mnesia_26 / pgsql_mnesia / 40e3f51 mssql_mnesia_26 / odbc_mssql_mnesia / 40e3f51 ldap_mnesia_25 / ldap_mnesia / 40e3f51 |
This PR addresses "Easier debugging if Mnesia is not configured, but some modules are configured with mnesia backend (or started dynamically in tests).
Proposed changes include:
Without it starts would "work", but we would get an error in case we wanna read/write into a table (which many modules ignore without writing to logs).