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

Support multiple HTTP inputs #6333

Closed
wants to merge 1 commit into from
Closed

Support multiple HTTP inputs #6333

wants to merge 1 commit into from

Conversation

gunnaraasen
Copy link
Member

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

Adds support for multiple HTTP listeners. This is the last issue to completely fix #2785.

@gunnaraasen
Copy link
Member Author

Looks like AppVeyor failed on an influx_stress test.

@e-dard
Copy link
Contributor

e-dard commented Apr 16, 2016

LGTM 👍

@jwilder
Copy link
Contributor

jwilder commented Apr 28, 2016

This is looks like a breaking change to the config file. Are there any other inputs that are limited to only one?

It would be nice to not have the config break. Can we keep the existing HTTP config option and add it to the slice for backwards compatibility?

@gunnaraasen
Copy link
Member Author

@jwilder I'm planning to open another PR to do exactly that for all the listeners but haven't finished yet.

@jwilder
Copy link
Contributor

jwilder commented Apr 28, 2016

@gunnaraasen What happens when you use an old config that still uses [http] and not [[http]]?

@@ -9,6 +9,7 @@
- [#5707](https://github.com/influxdata/influxdb/issues/5707): Return a deprecated message when IF NOT EXISTS is used.
- [#6334](https://github.com/influxdata/influxdb/pull/6334): Allow environment variables to be set per input type.
- [#6394](https://github.com/influxdata/influxdb/pull/6394): Allow time math with integer timestamps.
- [#6333](https://github.com/influxdata/influxdb/pull/6333): Support for multiple listeners for HTTP inputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be moved to next release.

@jwilder
Copy link
Contributor

jwilder commented May 13, 2016

This needs to be updated to support [http] and log a deprecation warning similar to how we handled [collectd] and [opentsdb]. See: https://github.com/influxdata/influxdb/blob/master/cmd/influxd/run/config.go#L133

@jwilder jwilder modified the milestones: 1.0.0 beta, 1.0.0 May 26, 2016
@jsternberg
Copy link
Contributor

What's the rationale behind this change? Unlike the other configuration changes, I don't see much value to multiple HTTP listeners. There are some downsides though with the naming behind the environment variable overrides and the configuration file change. If someone needs multiple HTTP listeners, what can we do better that Nginx can't do? Otherwise, I think we should just advise people to install a proper web server without risking the breaking change.

@jwilder
Copy link
Contributor

jwilder commented Jun 29, 2016

I agree w/ @jsternberg. I don't see the benefit of supporting this.

@gunnaraasen
Copy link
Member Author

Mainly, the benefit would be the ability to set up an unauthenticated localhost listener for administrative access. It would also make this service consistent with the Graphite, OpenTSDB, UDP, and CollectD services.

I'm going to close this since it's a large change not necessary for 1.0.

@gunnaraasen gunnaraasen deleted the ga-multiple-httpd branch April 20, 2018 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some services should be able to have multiple instances
4 participants