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

Revise default settings for Delayed Delivery #349

Closed
2 tasks done
bording opened this issue Feb 28, 2017 · 6 comments
Closed
2 tasks done

Revise default settings for Delayed Delivery #349

bording opened this issue Feb 28, 2017 · 6 comments
Labels
Milestone

Comments

@bording
Copy link
Member

bording commented Feb 28, 2017

#329 introduced two new settings to control the backwards compatibility of the delayed delivery feature, DisableTimeoutManager and AllEndpointsSupportDelayedDelivery. These settings default to being false to maximize compatibility with endpoints that haven't been upgraded yet.

We should consider revising the default values and/or removing these settings in the 5.0 time frame.

Plan of attack

@bording
Copy link
Member Author

bording commented Sep 20, 2017

The AllEndpointsSupportDelayedDelivery was removed because of #367, but we do have #370 to consider adding it back in some form.

@bording
Copy link
Member Author

bording commented Sep 23, 2017

Going to add this back to 5.0.0 for now so we can discuss options around how to achieve this, including making Disable/Enable mandatory now to enable an obsolete in the future.

@bording bording added this to the 5.0.0 milestone Sep 23, 2017
@adamralph
Copy link
Contributor

Discussed again today. One idea is the following:

In 5.0.0:

  • Introduce a new UseLegacyTimeoutManager(bool) (exact name TBD, perhaps UseHybridTimeouts or similar)
  • Throw an exception if the new method is not called

In 6.0.0:

  • Remove the exception throwing
  • Deprecate UseLegacyTimeoutManager(bool) with an error
  • Introduce a new EnableLegacyTimeoutManager() (or similar)

The problem with this solution is that it enforces the calling of a new method to every user of the transport. We need to give this some more thought before going ahead.

AllEndpointsSupportDelayedDelivery or its equivalent should be considered in a separate issue, if at all. It may not be worth pursuing that.

@adamralph adamralph removed this from the 5.0.0 milestone Oct 3, 2017
@bording
Copy link
Member Author

bording commented Dec 21, 2017

@Particular/rabbitmq-transport-maintainers I've been thinking about this one some again, and I think we might want to reconsider doing this as part of 5.0 after all.

@adamralph adamralph added this to the 5.0.0 milestone Dec 22, 2017
@adamralph
Copy link
Contributor

@bording I've added back to the 5.0.0 milestone to ensure that we at least consider it again.

@bording bording modified the milestones: 5.0.0, 5.1.0 Jan 2, 2018
@bording
Copy link
Member Author

bording commented Jan 22, 2018

The revised plan is to go ahead and change the behavior in 5.0.0 to disable the timeout manager by default, and add an API to opt-in to backwards compatible behavior.

We decided this was okay to do in a single major because if someone deployed without calling the new API, no messages in timeout storage would be lost. Adding the call and redeploying would be all that would be required to consume those messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants