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.
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)