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 command reply modes for replying from a given member #314

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Add command reply modes for replying from a given member #314

merged 2 commits into from
Nov 3, 2022

Conversation

the-mikedavis
Copy link
Member

Proposed Changes

This is a new effect which works the same as prior reply effects but is only executed by the given server. A caller may wish to block until a command has replicated to an arbitrary server, for example if that server is on the local node. This new reply mode ensures that the call to process the command will only return when the command is replicated to that given node.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • 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 existing functionality to not work as expected)
  • Documentation (correction or otherwise)
  • Cosmetics (whitespace, appearance)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • 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

@the-mikedavis the-mikedavis mentioned this pull request Oct 18, 2022
11 tasks
src/ra.erl Outdated Show resolved Hide resolved
src/ra_server_proc.erl Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis changed the title WIP Add a command reply mode for replying from a given node Add command reply modes for replying from a given member Oct 25, 2022
@the-mikedavis
Copy link
Member Author

I left off adding the new reply-modes to notifications since they're currently batched in a way that makes it a little tricky to separate by reply-mode. I can leave that to the future if we find the right use-case for them or tackle this in a separate PR. What do you think @kjnilsson?

@the-mikedavis the-mikedavis marked this pull request as ready for review October 25, 2022 21:30
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.

Looks great and I'd be happy to accept but have a question about the use of an options list and a couple of formatting comments.

src/ra_server_proc.erl Outdated Show resolved Hide resolved
src/ra_server.erl Outdated Show resolved Hide resolved
@@ -2307,6 +2318,15 @@ add_reply(_, '$ra_no_reply', _, Effects, Notifys) ->
{Effects, Notifys};
add_reply(#{from := From}, Reply, await_consensus, Effects, Notifys) ->
{[{reply, From, {wrap_reply, Reply}} | Effects], Notifys};
add_reply(#{from := From}, Reply,
{await_consensus, Options}, Effects, Notifys) ->
Replier = case Options of
Copy link
Contributor

Choose a reason for hiding this comment

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

it feels a bit strange to have a list but only evaluate the first item. I wonder if perhaps we should skip the list for now or use a map perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched to a map - if we need to add more options in the future I think it will be a better choice, and it's easier to work with than the list here since these options are mutually exclusive 👍

@kjnilsson
Copy link
Contributor

I left off adding the new reply-modes to notifications since they're currently batched in a way that makes it a little tricky to separate by reply-mode. I can leave that to the future if we find the right use-case for them or tackle this in a separate PR. What do you think @kjnilsson?

yes that is fine, would you mind creating an issue for this capturing some of the context you've got for the future?

We introduce two new command reply modes:

* `local`: reply to the sender of a command from a member on the same
  node as the sender of the command.
* `{member, Server}`: reply to the sender from the given member.
@kjnilsson kjnilsson merged commit 1e601b5 into rabbitmq:main Nov 3, 2022
@the-mikedavis the-mikedavis deleted the md-reply-from-node branch November 3, 2022 14:14
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.

2 participants