-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Graphite fixes3 #644
Graphite fixes3 #644
Conversation
should be a lot more efficient.
ok, forget about the articifial benchmarks. before 17:11 i was running stock 0.7.3, and trying to send 2k metrics/s in, but carbon-relay-ng had to drop most of it because influx couldn't handle it. at 17:11 i deployed this code live (this patchset applied on top of 0.7.3) at time of writing it's 17:25 carbon-relay-ng output:
pic that shows that before was almost all drops, and after, 100% working fine |
also fwiw, been running this code at vimeo for a while now, haven't found issues yet. |
Should this be reviewed and merged into master ? |
Yes please. It should be good. John Shahid notifications@github.com wrote:
|
break loop when channel closed, always also log shutdown so we know it worked
just noticed a bug: it doesn't break the commitloop in all cases when self.writeSeries gets closed, so the shutdown doesn't propagate properly. testing a fix right now.. EDIT: fixed |
(but not in the internal library, cause that's implicit)
note that the bug fixed by included commit 4f084d3 is also in the current master branch (i.e. in current master, connection is closed when it fails to process a line, which is not very good) |
} | ||
wg.Wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this loop ever break ? It looks like we will never get to the wg.wait()
line. Did I miss something ?
lgtm, other than the things noted in the comments |
otherwise this condition would be used no more than once and it would only flush based on the timer.
* break from TCP Accept() loop when connection closed, which was preventing shutdown to proceed * make sure that UDP functionality doesn't write to writeSeries channel after it has been closed. * clearer, more specific shutdown message in particular: * self.writers allows us to make sure things writing to writeSeries are done (they do blocking calls to handleMessage()) whether udp or tcp * self.connClosed lets us break from the Accept() loop, see http://zhen.org/blog/graceful-shutdown-of-go-net-dot-listeners/ (quit channel) * shutdown channel is now allCommitted things can get a little complicated, so here's a little schematic of how the functions and their logic relate: indent for a call out or important code within. everything shown as one nested tree server.go go ListenAndServe go committer reads from self.writeSeries until closed, then writes to self.allCommitted Serve for { Accept, breaks if err + connClosed self.writers.Add() go handleClient for { handleMessage reads until err and writes to self.writeSeries until read failed reads until EOF, ignores other handleMessage errors } conn.Close() self.writers.Done() } self.writers.Wait() close(self.writeSeries) Close() close(self.connClosed) self.conn.Close() wants confirmation on allCommitted channel; [timeout] returns within 5s
good to go. that travis build failure seems to be due to unrelated code. |
this is an updated version of #488
i have to test again to make sure, but i think it still performs worse than the original code (just like the 2 previous graphite-fixes PR's)