Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/ping response code #45

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kinok
Copy link

@kinok kinok commented Nov 15, 2016

Hey guys,

We had a problem regarding the hardcoded 204 No Content of /ping response code.
Indeed, Google Cloud Plateform http health-checks only accept 200 OK.
With this PR, you'll be able to control what response code of /ping you want/need to have, from the config file, using default-ping-response.

Thank you guys for this great tool!

Now you can decide what type of http status code the ping will return.
Copy link
Contributor

@joelegasse joelegasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple smaller changes I'd recommend

addr string
name string
schema string
response int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather see a name like pingResponseCode and have it separated by a newline so that it's visually separate from the above group of related fields.

@@ -113,10 +116,15 @@ func (h *HTTP) Stop() error {
func (h *HTTP) ServeHTTP(w http.ResponseWriter, r *http.Request) {
start := time.Now()

pingStatusResponse := DefaultHttpPingResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block should only happen once, and not on every request.

w.Header().Add("X-InfluxDB-Version", "relay")
w.WriteHeader(http.StatusNoContent)
return
w.Header().Add("X-InfluxDB-Version", "relay")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is going to return codes other than 204, Content-Length should also be set to 0.

@kinok
Copy link
Author

kinok commented Nov 16, 2016

Hey @joelegasse, thanks for your relevant review. I agree with all your remarks, just made the changes.

@kinok
Copy link
Author

kinok commented Mar 8, 2017

Hey there,
Any chance to have a review of this PR?
Thank you guys

@roman-vynar
Copy link

roman-vynar commented Sep 7, 2017

Also Amazon ELB requires 200 and does not work with 204.

@roman-vynar
Copy link

Getting an error with this PR:

goroutine 1 [running]:
github.com/influxdata/influxdb-relay/relay.NewHTTP(0xc420016761, 0xb, 0xc420014e21, 0xe, 0xc4200167e5, 0x0, 0x0, 0x0, 0xc8, 0xc42009c2c0, ...)
	/go/src/github.com/influxdata/influxdb-relay/relay/http.go:61 +0xdf
github.com/influxdata/influxdb-relay/relay.New(0xc4200561e0, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/influxdata/influxdb-relay/relay/relay.go:18 +0x15b
main.main()
	/go/src/github.com/influxdata/influxdb-relay/main.go:31 +0x1a1
panic: assignment to entry in nil map

Fix:

-       h := new(HTTP)
-
-       h.addr = cfg.Addr
-       h.name = cfg.Name
+       h := &HTTP{
+               addr:                cfg.Addr,
+               name:                cfg.Name,
+               pingResponseHeaders: map[string]string{},
+       }

@PatrickSplice
Copy link

Soooo...this is still an issue. Google gcp still only accepts 200 response codes. This has been open since 2016 and never fixed? It has been adjusted in both influxdb and telegraf. See this issue: https://github.com/influxdata/telegraf/pull/5704/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants