refactor #19
refactor #19
Conversation
|
work in progress... just pushing up what I had laying around locally... |
|
@kjk @shurcooL I've had this WIP refactoring PR open for a while (I've just rebased this on the deadlock fixes in #29) If you're interested/willing to help test/verify this beta code in high volume/concurrency situations, that would be helpful! I'd like to more aggressively work towards getting this tested and merged as I have some other public API refactoring that I think is long overdue (better sooner than later) cc @jehiah |
|
Currently the refactor_19 branch doesn't compile: ../../../bitly/go-nsq/reader.go:668: undefined: nsqConn |
|
fixed (posting from the right account this time) Also, in a separate issue I intend to document my proposal for improving the public facing API |
|
There's very verbose logging turned on (which seems to include hexdump of messages) compared to master (without me changing any flags), which makes debugging under heavy-load impractical. A sample verbose log message: It generated 4.3G of logs in few minutes. |
|
whooops, sorry about that, I'll remove that debug logging |
|
@kjk quieted the logging, let me know if the |
|
@mreiferson actually, |
|
It seems to be running fine against very heavy nsq traffic. It's not a deal breaker as it does work but looking at logs the shutdown sequence (i.e. calling I'm using my test program from https://gist.github.com/kjk/41cd49704c635b35bfd1. I use Full shutdown log is at https://gist.github.com/kjk/4401317d23f89f4ab3f3 A representitive sample: |
|
@kjk last commit should resolve those |
|
I'm still seeing e.g. "use of closed network connection" errors in the log: |
|
Hmmm, it's no longer erroring on closed connection while trying to I will continue to poke around, thanks. |
|
@kjk I fixed a few more I should really write tests 😈 |
|
This one does a clean shutdown - I don't see unexpected errors in the log. |
|
@kjk great! we'll clean this up and get it merged in, it's a much better foundation for the next round of changes I'm thinking of. thanks for all your help! |
| conn.OutputBufferTimeout = q.OutputBufferTimeout | ||
| } | ||
| conn.MessageCB = func(c *NSQConn, msg *Message) { | ||
| // log.Printf("[%s] EVENT: MessageCB %+v", c, msg) |
There was a problem hiding this comment.
commented out debug statements?
|
@mreiferson I'll put this out in some places on prod over the weekend, see if anything pops up. |
|
@jphines what's the deal |
| case string: | ||
| return strconv.ParseBool(v.(string)) | ||
| case int, int16, uint16, int32, uint32, int64, uint64: | ||
| return reflect.ValueOf(value).Int() == 0, nil |
There was a problem hiding this comment.
I think you might want != 0 for this?
There was a problem hiding this comment.
hah, good catch (that's currently broken in master)
|
was poking around here. ... and it barfed. hung up the entire reader. |
|
the host was also an nsqd instances AND lookupd fwiw |
|
hmm. looks like this locked up again. tracing log to see if I can find relevant information. |
|
|
* make Conn public * move transport responsibilities into Conn from Reader/Writer * supply callbacks for hooking into Conn events * name anon cleanup func; add to wg * ensure that readLoop exits before exiting cleanup * always check messagesInFlight at readLoop exit * close underlying connection last
|
This is holding up. |
this does essentially the same structural changes as pynsq in nsqio/pynsq#52