Skip to content
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

Don't set cluster_id if not needed #3112

Merged
merged 2 commits into from
May 14, 2021

Conversation

arcusfelis
Copy link
Contributor

This PR addresses "MIM-1396 Prepared query in mongoose_cluster_id causes errors on startup"

Proposed changes include:

  • Don't insert, if not needed.
  • Insert only when we generate a new ID.

Basically, we implement "select and just use it. If not found, create new and insert" logic.

It is a bit less robust:

  • if a cluster got started without RDBMS, but after it got a new node with RDBMS, we would not get an update in DB.
  • but this use case is pretty weird

There is still a race condition possible, when we try to insert, but someone has already inserted a ClusterID. we could handle it and retry, but it makes logic less clean. And I am pretty sure it's a rare case when two nodes go live for the first time. Could be avoided by adding retries, but nothing bad would happen anyway (we would see a warning in this case in logs, but that's it).

@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #3112 (6fb48f6) into master (9557370) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3112      +/-   ##
==========================================
- Coverage   79.22%   79.19%   -0.03%     
==========================================
  Files         388      388              
  Lines       31858    31864       +6     
==========================================
- Hits        25238    25235       -3     
- Misses       6620     6629       +9     
Impacted Files Coverage Δ
src/mongoose_cluster_id.erl 73.33% <80.00%> (-4.45%) ⬇️
src/ejabberd.erl 45.00% <0.00%> (-10.00%) ⬇️
..._distrib/mod_global_distrib_outgoing_conns_sup.erl 80.00% <0.00%> (-8.00%) ⬇️
src/cassandra/mongoose_cassandra_worker.erl 70.75% <0.00%> (-2.36%) ⬇️
src/global_distrib/mod_global_distrib_utils.erl 65.09% <0.00%> (-0.95%) ⬇️
src/mod_muc_log.erl 77.72% <0.00%> (ø)
src/ejabberd_c2s.erl 89.43% <0.00%> (+0.14%) ⬆️
...c/global_distrib/mod_global_distrib_server_mgr.erl 77.40% <0.00%> (+0.56%) ⬆️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 73.58% <0.00%> (+1.88%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9557370...6fb48f6. Read the comment docs.

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cleaner than before, the only issue is with the statement from the description:

we would see a warning in this case in logs, but that's it

This isn't completely true: we will get an error and a warning because of execute_successfully. I logged the original issue because the error was logged always, now it can happen sometimes, but I think it's acceptable, I just want to let you know. The upsert would eliminate it.

@chrzaszcz chrzaszcz merged commit 8c597bc into master May 14, 2021
@chrzaszcz chrzaszcz deleted the mu-no-cluster-id-duplicate-warning branch May 14, 2021 08:59
@Premwoik Premwoik added this to the 5.0.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants