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

Remove the "PreferBufferedTransport" flag #117

Merged

Conversation

AnthonySteele
Copy link
Contributor

@AnthonySteele AnthonySteele commented Oct 6, 2018

Summarise the changes this Pull Request makes.

The "PreferBufferedTransport" flag should always be true. In Version 4.0 we are are removing redundant code paths because

  • we can make breaking changes (although the majority of the consumers should be able to just recompile and carry on)
  • and these "experiments" can be productionised now. We keep the best/fastest path and drop the rest. The new code is ether ready for mainstream use, or should be made so.

So should the StatsDPublisher with an IStatsDPublisher _inner also go?
And the distinction between IStatsDTransport and IStatsDBufferedTransport ?

Please include a reference to a GitHub issue if appropriate.

The "PreferBufferedTransport" flag should always be true
@AnthonySteele AnthonySteele requested a review from a team as a code owner October 6, 2018 10:29
@martincostello martincostello changed the title Remove the "PreferBufferedTransport" flag [WIPRemove the "PreferBufferedTransport" flag Oct 6, 2018
@martincostello martincostello changed the title [WIPRemove the "PreferBufferedTransport" flag [WIP] Remove the "PreferBufferedTransport" flag Oct 6, 2018
@martincostello
Copy link
Member

Have added some musing on the above in #118.

@martincostello
Copy link
Member

So do you reckon this is ready to merge as-is, then I'll rebase #118 and merge that?

We can then do any allocation-savings once we've consolidated things a bit.

This was referenced Oct 7, 2018
@AnthonySteele
Copy link
Contributor Author

AnthonySteele commented Oct 7, 2018

If you want, we can merge this as is, and then consolidate after.
That bool option PreferBufferedTransport should go and the code should assume the true path, one way or the other.

@martincostello martincostello changed the title [WIP] Remove the "PreferBufferedTransport" flag Remove the "PreferBufferedTransport" flag Oct 7, 2018
@martincostello martincostello added this to the v4.0.0 milestone Oct 7, 2018
@martincostello martincostello merged commit 4822cb3 into justeattakeaway:master Oct 7, 2018
@AnthonySteele AnthonySteele deleted the buffer-on-by-default branch October 8, 2018 07:47
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