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

Disable HintedHandoff if configuration is not set. #4283 #4737

Merged

Conversation

ch33hau
Copy link
Contributor

@ch33hau ch33hau commented Nov 10, 2015

  • CHANGELOG.md updated
  • Rebased/mergable
  • Tests pass
  • Sign CLA (if not already signed)

Hi, I have created a pull request for #4283.

@corylanou
Copy link
Contributor

This looks good. However, if we are changing the config here to be disabled by default, we need to update the DemoConfig to turn it on here:

https://github.com/influxdb/influxdb/blob/master/cmd/influxd/run/config.go#L101

c.HintedHandoff.Enabled = true

@otoolep
Copy link
Contributor

otoolep commented Nov 10, 2015

Not sure about this. Since HH is on by default, with no config section,
shouldn't it still be on?

On Tuesday, November 10, 2015, Cory LaNou notifications@github.com wrote:

This looks good. However, if we are changing the config here to be
disabled by default, we need to update the DemoConfig to turn it on here:

https://github.com/influxdb/influxdb/blob/master/cmd/influxd/run/config.go#L101

c.HintedHandoff.Enabled = true


Reply to this email directly or view it on GitHub
#4737 (comment).

@ch33hau
Copy link
Contributor Author

ch33hau commented Nov 10, 2015

@corylanou ok, sure
@otoolep Previously yes, it still cause the Dir validation failed. So after this fixes, enabled will be false if no config section or we set it to false.

@ch33hau ch33hau force-pushed the 4283-hh-throws-error-even-if-disabled branch from 07f74fa to 8bfdfbd Compare November 10, 2015 17:13
@corylanou
Copy link
Contributor

@otoolep We ship everything with a sane config, including running in demo mode with it turned on. The only way for HH to be turned off is by the end user removing the config for it. We have other sections that work the same way. Thoughts?

@otoolep
Copy link
Contributor

otoolep commented Nov 11, 2015

OK, I checked https://github.com/influxdb/influxdb/blob/master/etc/config.sample.toml#L99 and we have not been giving the impression that HH is on by default. So I'm OK with this change.

I was concerned that HH was documented as on by default in the sample config, and then this could be considered a breaking change. This is not the case, and I actually agree it is more natural if the section is not there, one shouldn't be surprised if HH is not on.

@otoolep
Copy link
Contributor

otoolep commented Nov 11, 2015

+1 from me.

@corylanou if +1 from, please merge too.

Thanks @ch33hau

@corylanou
Copy link
Contributor

+1

corylanou added a commit that referenced this pull request Nov 11, 2015
…sabled

Disable HintedHandoff if configuration is not set. #4283
@corylanou corylanou merged commit 6ecb62e into influxdata:master Nov 11, 2015
@ch33hau
Copy link
Contributor Author

ch33hau commented Nov 11, 2015

Thanks for review and comments @otoolep @corylanou !

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.

None yet

3 participants