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

Add federation support for quorum queues #2804

Merged
merged 4 commits into from
Feb 25, 2021
Merged

Conversation

dcorbacho
Copy link
Contributor

Adds decorator support for quorum queues, in order to support the federation plugin. There are no changes on the federation plugin, but the queue specific test suite uses now both classic and quorum queues.

This is part of #2756, which will be finished when this is backported to master. Small changes are needed to apply this patch in master, due to the queue type refactor.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • 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

@dcorbacho dcorbacho marked this pull request as draft February 10, 2021 14:19
deps/rabbit/src/rabbit_fifo.erl Show resolved Hide resolved
@@ -456,11 +460,13 @@ apply(#{system_time := Ts} = Meta, {down, Pid, noconnection},
% Monitor the node so that we can "unsuspect" these processes when the node
% comes back, then re-issue all monitors and discover the final fate of
% these processes

NotifyEffect = notify_decorators_effect(State),
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the scenario we are notifying here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consumers are marked as suspected down, so it could end up with no active consumers at all and that needs to be notified.

{State1, _Result, Effects1} = checkout0(Meta, checkout_one(Meta, State0),
Effects0, {#{}, #{}}),
case evaluate_limit(Index, false, OldState, State1, Effects1) of
{State, true, Effects} ->
update_smallest_raft_index(Index, State, Effects);
case have_active_consumers_changed(OldState, State) of
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about adding more code this very hot path. is there nowhere else we an do this and achieve the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to detect changes in the active consumers, unless we keep track of them on a different way for this exclusive use I'm not sure how we can do it

Copy link
Member

Choose a reason for hiding this comment

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

If we don't do this here, what would be the observable effects? If it's a minor delay in consumer topology changes, we can live with that over more code executed on the hot path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the notifications for enqueue/return/credit/settle/discard operations. It seems enough for federation purposes (our only decorator?) to only notify on checkout/node up/node down/config updates. At least, federation test suite still passes.

deps/rabbit/src/rabbit_quorum_queue.erl Show resolved Hide resolved
@dcorbacho dcorbacho marked this pull request as ready for review February 15, 2021 16:09
Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

Right now policies that include federation-related keys are not applied to quorum queues.

dcorbacho added 3 commits February 18, 2021 17:15
It's not possible to know all aplicable policies since plugins can extend
these, i.e. federation. Thus, we'll exclude the known unapplicable core policies
and allow through any other policy.
Copy link
Contributor

@kjnilsson kjnilsson left a comment

Choose a reason for hiding this comment

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

The function naming is the main thing I think we should clarify before merging.

deps/rabbit/src/rabbit_fifo.erl Outdated Show resolved Hide resolved
deps/rabbit/src/rabbit_fifo.erl Show resolved Hide resolved
deps/rabbit/src/rabbit_quorum_queue.erl Outdated Show resolved Hide resolved
@michaelklishin michaelklishin merged commit a2f98f2 into master Feb 25, 2021
@michaelklishin michaelklishin deleted the rabbitmq-server-2756 branch February 25, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants