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

Do not confirmSelect more than once per channel #1057

Merged

Conversation

sergio91pt
Copy link
Contributor

Proposed Changes

In order to avoid unnecessary blocking RPC calls and conform to best practices, the Channel now checks if it already activated confirm mode before sending a confirm.select RPC call.

If confirm mode is already activated, calling confirmSelect() again returns immediately without sending an RPC call.

Closes #1056

Types of Changes

  • Bugfix (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

  • 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

In order to avoid unnecessary blocking RPC calls and conform to best
practices, the Channel now checks if it is already activated confirm
mode before sending a confirm.select RPC call.

If confirm mode is already activated, calling confirmSelect() again
returns immediately without sending an RPC call.

Closes rabbitmq#1056
@michaelklishin
Copy link
Member

In a design briefly discussed on our internal Slack with @acogoluegnes, we concluded that it would be idea to use an AtomicBoolean. On the other hand, channels are not mean to be shared between threads as much as possible, and never for publishers. So this PR looks OK as it is.

@acogoluegnes
Copy link
Contributor

It's an optimization for a network that is already idempotent and channels are not meant to be used by several threads, so we can save a bit of memory by using a boolean. We could also make the property volatile, this should have an impact only on calls to confirmSelect(). I'd lean towards the volatile boolean, but I'm not super opinionated about it.

@acogoluegnes
Copy link
Contributor

@sergio91pt Can you increase the timeout in the test, to 10 seconds? It passes locally, but not on CI.

@sergio91pt
Copy link
Contributor Author

@acogoluegnes Can you run the pipeline again? I refactored the test and it should pass now. I already run it 3000 times through the IDE and it doesn't seem flaky anymore.
It now uses a thread, since I noticed that convenient Executor was actually being passed to the code under test, so I shouldn't be using it 😄

Regarding the boolean, I also was considering a volatile boolean initially but the usage of nextPublishSeqNo made it clear this method is not intended to be thread safe, so I ended leaving it as a normal boolean.

@michaelklishin michaelklishin merged commit ab0c62d into rabbitmq:main Jun 13, 2023
7 checks passed
@acogoluegnes acogoluegnes added this to the 5.18.0 milestone Jun 13, 2023
@sergio91pt sergio91pt deleted the confirm-select-once-per-channel branch June 13, 2023 11:02
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.

Do not confirmSelect more than once per channel
3 participants