Skip to content

Instantly share code, notes, and snippets.

@schmichael
Last active August 29, 2015 13:57
Show Gist options
  • Save schmichael/9745170 to your computer and use it in GitHub Desktop.
Save schmichael/9745170 to your computer and use it in GitHub Desktop.
Thoughts on go-nsq
  • I don't understand why you'd want more than one type of Handler per Reader, so why not add Handler to Reader's constructor with an optional concurrency int on Reader?
  • 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 the ExitChan (which should be a struct{})? Why not simply make Stop block until handlers have exited gracefully?
  • Reader.Configure is awkward, but makes sense if you want to provide validation for Reader 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 (like tls.Config) which could be built as a struct literal and passed to Reader.Configure (or a constructor?) would allow for validation and public fields.
  • Proper Reader initialization is non-obvious. I think the order is:
  1. r := NewReader()
  2. r.AddHandler(h)
  3. r.ConnectToLookupd(...)
  4. <-r.ExitChan # probably select on signals here as well
  • Reader and Writer are confusing names since they don't implement the expected interfaces. Consumer and Publisher 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 separate Readers 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 efficient ByteOrder 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.

nsqd

  • GET /empty_channel returns a 500 on unknown topics, should be a 400 or 404.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment