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

Using async queue instead of submit new Runnable each time #25

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

maurofran
Copy link

Looking at the code, I noticed that every time a message is sent to the statsd server, a new Runnable instance is created. I think better performance can be achieved using a single Runnable and a Queue in which messages are put, waiting to be sent to the statsd server.

@scarytom
Copy link
Contributor

Thanks for submitting this. I was thinking of doing something similar myself as part of issue #16.

Would it be possible for you to force-push a better pull request without tangential changes to the signature of messageFor (where you have changed value back to being an Object) and leaving (Locale)null for the String.format call. A lot of your commits seem to make changes then reverse them, so it would be good if you could flatten the commits, rebase, then force-push back.

I'm happy to review and consider pulling your request, but I don't like multiple things changing at once. I haven't looked in depth yet, but by using a BlockingQueue, you are probably violating the non-blocking contract of this library.

@maurofran
Copy link
Author

Yes... I'have done few experiments (one used in order to also use StringBuilder instead of String.format) but it breaks the unit tests.
I will make the changes you ask for in the next days: I will make a single pull request with only the changes using the Queue.

@scarytom
Copy link
Contributor

OK, Thanks.

Do have a check before you start, as I may make some changes for #16 in the next few days.

@michaelsembwever
Copy link

A faster approach is using the lmax-disruptor

See finn-no@5b0e893

Or the branch for this work, https://github.com/finn-no/statsd-lmax-disruptor-client/tree/feature/lmax-disruptor

we've been using this in production at Norway's busiest website for 5 months now.

@scarytom
Copy link
Contributor

I'm not sure I want to introduce a dependency on a 3rd party library like the lmax-disruptor. I like to think of the java-statsd-client as a small self-contained library without dependencies outside core Java.

@waynr
Copy link

waynr commented Sep 21, 2016

https://github.com/DataDog/java-dogstatsd-client appears to be a more actively maintained version of this project, I recommend submitting your patch there if you have not already done so.

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.

6 participants