- I don't understand why you'd want more than one type of
Handler
perReader
, so why not addHandler
toReader
's constructor with an optional concurrencyint
onReader
? - A
HandlerFunc
function type would be a nice alternative interface for stateless handlers or closures. - Graceful shutdown is confusing and awkward. A (non-blocking) 30 second sleep in
Stop
must be followed by blocking on theExitChan
(which should be astruct{}
)? Why not simply makeStop
block until handlers have exited gracefully? Reader.Configure
is awkward, but makes sense if you want to provide validation forReader
fields. However, it seems strange to make the fields public and provide a setter with no indication which is the proper mechanism to use.- Perhaps a separate
ReaderConfig
type (liketls.Config
) which could be built as a struct literal and passed toReader.Configure
(or a constructor?) would allow for validation and public fields. - Proper
Reader
initialization is non-obvious. I think the order is:
r := NewReader()
r.AddHandler(h)
r.ConnectToLookupd(...)
<-r.ExitChan # probably select on signals here as well
-
Reader
andWriter
are confusing names since they don't implement the expected interfaces.Consumer
andPublisher
might be more clear (and could be in separate packages since they're almost always used independently). -
I expected the
AddHandler
interface to allow me to subscribe to multiple nsq channels within a single reader. Managing separateReader
s makes getting graceful shutdown right even trickier. -
Use of Go's builtin
log
package is frustrating (very noisy during integration tests) but understandable. Not sure if there's a better answer here. -
DecodeMessage
should use the more efficientByteOrder
methods: http://golang.org/pkg/encoding/binary/#ByteOrder -
Unnecessary slice: https://github.com/bitly/go-nsq/blob/master/message.go#L83- It is necessary, Id is a byte array.
GET /empty_channel
returns a 500 on unknown topics, should be a 400 or 404.