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.

Sign up for GitHub

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

Jump to bottom

Partially reintroduce locking to mirrored_supervisor #3263

Merged
merged 4 commits into from
Aug 5, 2021
Merged

Partially reintroduce locking to mirrored_supervisor #3263

merged 4 commits into from
Aug 5, 2021

Conversation

michaelklishin
Copy link
Member

@michaelklishin michaelklishin commented Aug 4, 2021 •
edited
Loading

Proposed Changes

Unlike the now removed pg2 module, pg in Erlang 24 does not acquire locks when group
membership changes. It is strongly eventually consistent by design. mirrored_supervisor
implicitly relied on the pg2 locking behavior and without it, duplicate children can
be started in some scenarios.

This reintroduces some of the locking performed by pg2. For the use cases of mirrored_supervisor,
locking across all connected nodes is acceptable.

Types of Changes

  • Bug fix (non-breaking change which fixes issue Duplicate dynamic shovels can be started in certain scenarios #3260)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

Closes #3260. References #3132, #3154.

@michaelklishin
Mirrored supervisor: more logging at debug level
4f3d5e5
@michaelklishin
More mirrored supervisor logging
9a0f4b1
@michaelklishin
Acquire locks when starting mirrored supervisor children
22add3c 
Unlike pg2, pg in Erlang 24 is eventually consistent. So this
reintroduces some of the same kind of locking mirrored_supervisor
used to rely on implicitly via pg2.

Per discussion with @lhoguin.

Closes #3260.

References #3132, #3154.
lhoguin
lhoguin approved these changes Aug 5, 2021
View reviewed changes
Copy link
Contributor

@lhoguin lhoguin left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

@michaelklishin michaelklishin merged commit cf2c609 into master Aug 5, 2021
@michaelklishin michaelklishin deleted the rabbitmq-server-3260 branch August 5, 2021 12:33
@michaelklishin
Copy link
Member Author

@Mergifyio backport v3.9.x v3.8.x

This was referenced Aug 5, 2021
Closed
Closed
@mergify
Copy link

mergify bot commented Aug 5, 2021

Command backport v3.9.x v3.8.x: success

Backports have been created

@michaelklishin
Copy link
Member Author

Note: v3.8.x requires conflict resolution and some massaging to compile on Erlang 24 (it relies on Lager) => will backport manually.

michaelklishin added a commit that referenced this pull request Aug 5, 2021
@michaelklishin
Merge pull request #3263 from rabbitmq/rabbitmq-server-3260
410925b 
Partially reintroduce locking to mirrored_supervisor

(cherry picked from commit cf2c609)

Adapted for Erlang 24 compatibility of rabbit_log call sites.

Conflicts:
	deps/rabbit_common/src/mirrored_supervisor.erl
@michaelklishin
Copy link
Member Author

Backported to v3.8.x.

michaelklishin added a commit that referenced this pull request Aug 5, 2021
@michaelklishin
Merge pull request #3263 from rabbitmq/rabbitmq-server-3260
cdb0a52 
Partially reintroduce locking to mirrored_supervisor

(cherry picked from commit cf2c609)
@michaelklishin
Copy link
Member Author

Backported to v3.9.x.

@michaelklishin michaelklishin mentioned this pull request Aug 8, 2021
Closed
@johanrhodin johanrhodin mentioned this pull request Aug 12, 2021
Closed
This was referenced Aug 13, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binarin binarin binarin left review comments

@lhoguin lhoguin lhoguin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate dynamic shovels can be started in certain scenarios
3 participants
@michaelklishin @binarin @lhoguin