Read node_id from discovery_nodes table in RDBMS #4042
Conversation
Report stacktrace if we fail to start the application
Start mongoose_start_node_id
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feature/cets #4042 +/- ##
================================================
+ Coverage 82.20% 82.21% +0.01%
================================================
Files 539 541 +2
Lines 33952 34012 +60
================================================
+ Hits 27909 27962 +53
- Misses 6043 6050 +7
☔ View full report in Codecov by Sentry. |
arcusfelis
force-pushed
the
feature-cets-node_id
branch
from
173a773
to
136aff5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There are two backends: Mnesia or CETS disco. If none backends are useful, we fallback to node_id to be 0 (which is not that major issue for MAM ID collisions)
arcusfelis
force-pushed
the
feature-cets-node_id
branch
from
136aff5
to
e1b7ad2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
We don't care if test2 is returned in a list returned on test2 node
This comment was marked as outdated.
This comment was marked as outdated.
Fixes mongoose_cleanup_SUITE:stream_management stop logic
This comment was marked as off-topic.
This comment was marked as off-topic.
No unsigned type
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
arcusfelis
force-pushed
the
feature-cets-node_id
branch
from
4229127
to
4bff87e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
small_tests_24 / small_tests / 8978847 small_tests_25_arm64 / small_tests / 8978847 small_tests_25 / small_tests / 8978847 ldap_mnesia_24 / ldap_mnesia / 8978847 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 8978847 ldap_mnesia_25 / ldap_mnesia / 8978847 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 8978847 dynamic_domains_mysql_redis_25 / mysql_redis / 8978847 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 8978847 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 8978847 internal_mnesia_25 / internal_mnesia / 8978847 pubsub_SUITE:dag+node_config:retrieve_configuration_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}]},
{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}]}]}}pgsql_mnesia_24 / pgsql_mnesia / 8978847 mysql_redis_25 / mysql_redis / 8978847 pgsql_cets_25 / pgsql_cets / 8978847 pgsql_mnesia_25 / pgsql_mnesia / 8978847 mssql_mnesia_25 / odbc_mssql_mnesia / 8978847 |
There was a problem hiding this comment.
It's looking good in general. I added a few comments, and I have a question: I understand that we don't need numbers in ejabberd_local but what's wrong about using them, and thus avoiding the need to have two kinds of node id's? Is your intention to actually enforce a fresh id on each node restart, so that we don't try to handle a response for a request sent by a node that was already restarted?
|
Main motivation was to still able to use ejabberd_local with test presets that do not have rdbms or mnesia. So, decided to split responsibilities. Automatic cleaning on node restart comes for free in this case. I was worried a bit about conflict resolution strategy for node nums. Because changing node num in runtime would not affect any MAM logic. But for iqs routing it will break not finished replies, so decided a spend a bit of time to make iq routing not failing for sure. |
|
small_tests_25_arm64 / small_tests / b83dc98 small_tests_24 / small_tests / b83dc98 small_tests_25 / small_tests / b83dc98 ldap_mnesia_24 / ldap_mnesia / b83dc98 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / b83dc98 ldap_mnesia_25 / ldap_mnesia / b83dc98 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / b83dc98 pgsql_mnesia_24 / pgsql_mnesia / b83dc98 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / b83dc98 dynamic_domains_mysql_redis_25 / mysql_redis / b83dc98 pgsql_cets_25 / pgsql_cets / b83dc98 internal_mnesia_25 / internal_mnesia / b83dc98 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / b83dc98 mysql_redis_25 / mysql_redis / b83dc98 pgsql_mnesia_25 / pgsql_mnesia / b83dc98 mssql_mnesia_25 / odbc_mssql_mnesia / b83dc98 |
This PR addresses MIM-1916
Task: MongooseIM’s ejabberd_node_id should work without Mnesia.
As a part of adding support to disable Mnesia, we need to store node_id’s somewhere.
Currently it is a simple file with only Mnesia support.
We use node_id’s in two places:
MAM, to avoid collisions in the mam ids.
we now
use src/mongoose_node_num.erlfile. The node id is set from mnesia or from rdbms. It is not that critical if it remains 0 though, collisions for MAM ID inside one archive are not allowed, but extremely unlikely.local IQs module, so we know which node generated which IQ request, when handling IQ result that came from s2s, for example. We don’t need numbers here, we could generate a random uuid on the node start and store it across the cluster instead
check src/mongoose_start_node_id.erl module for the logic
Change name for ejabberd_node_id - DONE (there were no ejabberd_node_id module in ejabberd, it was added in mongooseim during MAM work just the name was following the naming convention back than - i.e. every non-gen-module code has this prefix to avoid name clashing).
Maybe just adding an auto-incremented key to the discovery_nodes table is an option - DONE, but we also check for gaps now.