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 diffSample optimisation #17

Merged
merged 2 commits into from
Sep 25, 2017

Conversation

adinapoli-iohk
Copy link
Contributor

@adinapoli-iohk adinapoli-iohk commented Sep 20, 2017

@23Skidoo @tibbe This PR removes the diffSample optimisation to allow statsd to pick us these "low-level" details for us, as client code shouldn't be bothered.

See here for a better description and the rationale around this PR: #16

@adinapoli-iohk
Copy link
Contributor Author

The Travis error seems to be a red herring:

Errors were encountered while processing:
 /var/cache/apt/archives/cabal-install-head_2.1+git20170813.0.267efc8-4~14.04_amd64.deb
 /var/cache/apt/archives/ghc-8.2.1_8.2.1-2~14.04_amd64.deb
W: --force-yes is deprecated, use one of the options starting with --allow instead.
E: Sub-process /usr/bin/dpkg returned an error code (1)

@tibbe
Copy link
Owner

tibbe commented Sep 20, 2017

Probably goes without saying, but please do test these changes against statsd to make sure you get the expected behavior.

@adinapoli-iohk
Copy link
Contributor Author

Sounds like a plan. I'm not expert enough with statsd to judge whether or not this PR is solving our woes, but let me get back to you once we have done an internal run with the feature branch this PR was created from. cc @domenkozar

Copy link
Collaborator

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please add a note to the changelog?

@domenkozar
Copy link

@adinapoli-iohk let's do an upstream stack change to use your branch and we'll deploy that to staging to verify.

@adinapoli-iohk
Copy link
Contributor Author

@domenkozar Absolutely.

LGTM. Can you please add a note to the changelog?

Sure thing, leave it with me. I will confirm this is working as intended with Domen and will update the changelog accordingly. Which version should I put into the changelog? CURRENT_VERSION + 1 I guess ?

@23Skidoo
Copy link
Collaborator

0.2.2.0, I think: even though this change is backwards-compatible, it changes the behaviour, so it should be possible to guard against it with a MIN_VERSION_ macro.

@domenkozar
Copy link

I can confirm we see multiple consequent same-value pushes to statsd using this PR.

@adinapoli-iohk
Copy link
Contributor Author

@domenkozar Neat! I guess now it's my turn to update the changelog so that we can make the PR officially mergeable 😛

@23Skidoo 23Skidoo merged commit e444d8b into tibbe:master Sep 25, 2017
@23Skidoo
Copy link
Collaborator

Merged, thanks!

@23Skidoo
Copy link
Collaborator

ekg-statsd-0.2.2.0 is now on Hackage.

@adinapoli-iohk adinapoli-iohk deleted the drop-diff-optimisation branch September 25, 2017 15:49
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

4 participants