nsq_to_file: require --topic or --topic-pattern

[judwhite]

fixes #789

Notes:

• It's a little odd you can specify --topic-pattern without --topic only when using --lookupd-http-address; seems like this should work for --nsqd-tcp-address too.
• While testing I accidentally ran nsq_to_file --topic [topic1] [topic2] ..., which didn't produce an error, but I wondered why topic2 wasn't getting picked up.
• Thought (too much) about the if pattern == "" in allowTopicName. I think the alternative is to set it to .* if it's empty once we've determined len(topics) > 0 in main(). I don't have a strong opinion so leaving it to review.

Release notes should indicate people previously using default settings to archive all topics will need to change their command line args.

RFR /cc @mreiferson @jehiah @elubow

[ploxiln]

I think there's a bit of a problem with changing the default topic-pattern without changing this part:

 for _, topic := range topics {
        if !discoverer.allowTopicName(*topicPattern, topic) {
            log.Println("Skipping topic ...")

EDIT: oh I see you've mentioned this explicitly. I guess I also mentally tripped on this a bit. nvm.

👍 for this very minimal behavior fix

[mreiferson]

It's a little odd you can specify --topic-pattern without --topic only when using --lookupd-http-address; seems like this should work for --nsqd-tcp-address too.

Agreed, would just need to be implemented differently.

While testing I accidentally ran nsq_to_file --topic [topic1] [topic2] ..., which didn't produce an error, but I wondered why topic2 wasn't getting picked up.

That's a Go flag parsing thing and I don't think users expect it to work that way (since it implements pretty typical linux-y style), but I could obviously be wrong.

Thought (too much) about the if pattern == "" in allowTopicName. I think the alternative is to set it to .* if it's empty once we've determined len(topics) > 0 in main(). I don't have a strong opinion so leaving it to review.

If it's confusing let's clean it up?

[judwhite]

If it's confusing let's clean it up?

Do you think it is? I really have no strong opinion, let me know if you want it changed.

[mreiferson]

After looking at it in context, I think both approaches lack obviousness, and I don't have a simple solution. At a high level, I think what makes the most sense is for explicit topics to never go through the discovery code path.

Related, I've always been unhappy with the fact that the topic discovery code is copied in two places.

[judwhite]

Yeah I think my "no strong opinion" stems from the fact I don't like either option 😄 Maybe something closer to right after flag.Parse is called would be more obvious.

I do like the idea of removing the pattern match when explicit topics are set... if you have both --topic and --topic-pattern set we can kick them right out.

[ploxiln]

That's a Go flag parsing thing and I don't think users expect it to work that way (since it implements pretty typical linux-y style)

I don't think it's normal for an unrecognized argument to be silently ignored

(... except for uncommon cases like "true" which don't ever inspect arguments)

[mreiferson]

I don't think it's normal for an unrecognized argument to be silently ignored

Fair point, it probably would make sense for it to error. From the docs:

Flag parsing stops just before the first non-flag argument ("-" is a non-flag argument) or after the terminator "--".

[mreiferson]

I guess it considers those "arguments"?

[mreiferson]

is this ready to go?

[judwhite]

@mreiferson for the initial request it is. if we want to do the rest of the suggestions discussed we can wait, and make the code a little better. been busy but things should settle shortly.

[mreiferson]

@judwhite if this addresses the original issue, let's land it, and we can follow up with cleanup if we have the time.