nsqd: http api inconsistencies

[mreiferson]

It's a little weird to use GETs, but the docs don't mention which verb to use. It would be nicer to have DELETE host/topic/test or the POST at least just for consistency, POSTS are definitely more curl-friendly so maybe going full "rest" (meh) is not worth it

curl -d topic=test http://127.0.0.1:4151/delete_topic
{"status_code":500,"status_txt":"MISSING_ARG_TOPIC","data":null}

curl http://127.0.0.1:4151/delete_topic?topic=test
{"status_code":200,"status_txt":"OK","data":null}

[tj]

Another small thing: responds with 500 for an invalid topic, could be a 400

[mreiferson]

Yea, this stuff could certainly be improved, thanks.

I'm going to clump this into the proposed work to version the HTTP endpoints that came from the other issue you reported in #308, we can do this work at the same time.

[tj]

Cool if I get some time I'll try and help with these, another really minor thing is supporting Accept over (or in addition to) ?format would be nice

[mreiferson]

Ok, pushed some code for review... If anyone else has opinions speak up now!

This implements:

• versioning
• name spacing
• mutations require POST
• arguments still in request params because it's easier to curl on CLI and it would show up in logs
• better HTTP status codes and removes redundant envelope

cc @jehiah

[tj]

LGTM!

[mreiferson]

thanks, also in case it wasn't obvious this change preserves all of the existing endpoints and semantics, to be officially dropped in 1.0

[mreiferson]

RFR @jehiah

[jehiah]

i had some of this conversation with @mreiferson offline, but the gist is:

If we want to migrate our public api to a new response format, and provide a transition period for clients to opt-in to the new format we could do that with Accept: ... header.

This seems more appealing as we would have a time where endpoints would respond in either format, clients could update to opt in to the new format, then we can drop the old format. This also provides a path if/when we need to make backwards incompatible changes on a single endpoint and allows us to do so without ending up with a single endpoint in /v2/ while others are in /v1/. (clearly request parameters could also be used for such a purpose).

There is also an appealing part about using Accept header in that a client can opt in to the new format, but can also still talk to old endpoints fairly gracefully to allow for a slow migration.

[jehiah]

missing info

[mreiferson]

was intentional, /v1/stats returns the version - seemed superfluous

[mreiferson]

Anyone else care to weigh in?

I don't really care much whether we use a prefix v1/ or Accept header, you make good arguments for going with Accept.

I'm mostly interested in the response format change and the other endpoint renaming changes I did here, which we can do regardless of the choice for specifying a version.

[tj]

I'm impartial, I don't think either is inelegant, pathname is nice and curl-able that's about it

[mreiferson]

updated for Accept @jehiah

[mreiferson]

@jehiah ping

[jehiah]

LGTM you want this in as is?

[mreiferson]

nah, I'll squash

[mreiferson]

ok, ready

[jehiah]

@mreiferson it occurs to me that we missed updating api endpoints in nsqlookupd w/ this new response format. Care to open up a PR to match response handling there?

[mreiferson]

@jehiah good call, nsqadmin as well... will do

[jehiah]

I didn't think nsqadmin had any api responses, but i see two spots that use that formatting.

[mreiferson]

I was more referring to its interaction with nsqlookupd and nsqd (to move to using the newer response format)

[jehiah]

speaking for my own ease of deploying, i'd like to get a version of nsqd build and deployed w/ new response formats available prior to the switch to use them from nsqadmin. (or i suppose the flip side is having it still fall back to accept the old response type)

[mreiferson]

It'll be backwards compatible (just prefer the new format)