TODO pending judwhite/go-svc#15

Callers of GetTopic() previously expected a non-nil response. It seems like this check is to prevent any of the code paths that create topics as a side effect from recreating topics during drain.

Would it be better to just bite-the-bullet and convert this to return an error rather than retrofitting this into a nil response?

Alternatively, we could add an nsqd.IsTopicDraining() func and the call sites that create topics as side effects can check it before calling GetTopic()?

I generally agree this is the most non-obvious bit of logic - I'll start by renaming these to GetOrCreate.... and docstring that creation may not succeed.

I think despite being atomic we want the isDraining here in the read lock so this can't race a concurrent nsqd starting to drain.

Good point on the race. Still, we shouldn't abuse nil here. This is a new edge case where a topic won't be returned and we have to modify all call sites anyway to check for nil, might as well do it "right" and add a proper error return value?

PTAL - I think the rename to GetOrCreate{Topic,Channel} helped a lot; I also documented that these may or may not succeed. Given that, checking nil feels an idiomatic way to implement.

We could switch but feels like bike shedding? The rename was helpful in ensuring i've covered all code paths (a few more were updated) - i should have done a more exhaustive check originally.

Probably worth benchmarking... but we've tried hard to minimize the logic in the core message loop. In this case, draining is an irreversible state, meaning we just need to propagate the state change locally into this goroutine (e.g. like paused state) rather than check an atomic for every message published.

I tried to write some benchmarks generally for messagePump but I couldn't get a consistent benchmark output. (as such i'll probably drop that benchmark from this PR before landing - unless it looks useful). In trying to benchmark i ran into issues with GUID sequences expiring so I switched some benchmarks to use an integer sequence to avoid timing impacts from GUID sequences expiring.

name                old time/op   new time/op   delta
TopicMessagePump-2  3.06µs ±181%  7.04µs ±131%   ~     (p=0.113 n=9+10)

I'd assume a refactor which added another 'drainChanchannel forTopic.messagePumpto watch for draining state updates would be more expensive thanatomic.LoadInt32` but.. i'm not sure - @ploxiln have any intuition here?

There's a race condition b/w this line and line 162 (a concurrent pub that had already passed the isDraining check but lost the race w/ line 162)

Naming bla bla bla... do we need the /state prefix?

From an API layout (and documentation) perspective I felt it was good to have these grouped together, but i'm also not sold on the /state prefix; Any other naming ideas?

What's up with these?

Since this is a HTTP request, there is some desire to allow the HTTP request to complete (i.e. the caller should get a HTTP response back) before the action takes place. If we call immediately an shutdown can (and will in many cases) close this connection and the caller will get a connection error.

I'm open to other ways to solve this (because it's obviously not a generally good pattern to rely on timing), but this seemed reasonable for the scope of this endpoint.

I don't think we need these changes in LoadMetadata. It is only ever called from Main(), so it's impossible for it to encounter topics/channels that are being drained.