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

[core] for settings that take units (byte size, time), we should require a unit #7616

Closed
mikemccand opened this issue Sep 5, 2014 · 5 comments · Fixed by #11437

Comments

@mikemccand
Copy link
Contributor

commented Sep 5, 2014

Too many users have fallen into the trap of thinking refresh_interval of "1" means "1s" but in fact it means "1ms" ... I think it should be a hard error if this setting (and maybe any setting that takes units) is missing the units?

We should require that the unit is explicit so users don't fall into this trap?

@mikemccand mikemccand self-assigned this Sep 5, 2014
@kimchy

This comment has been minimized.

Copy link
Member

commented Sep 5, 2014

+1, we should do it globally on the Settings class to begin with (with relevant flags on the relevant type parsers). I am unsure about breaking backward comp. for this, we could check in the settings for a specific setting, and if its set, still allow non unit settings? This will allow users to revert to the old behavior until they figure out how to change them. Some settings might not be dynamic to update on the index level...

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2014

That's a good point @kimchy, would not be nice to make this a hard break when it's a write-once setting.

So maybe we default to "strict" but add a back-compat setting that user could enable if it's too hard / not possible to add units to their existing settings?

@kimchy

This comment has been minimized.

Copy link
Member

commented Sep 5, 2014

@mikemccand make sense to me

@clintongormley

This comment has been minimized.

Copy link
Member

commented Sep 6, 2014

+1

@mikemccand mikemccand added the >breaking label Sep 7, 2014
@mikemccand mikemccand changed the title For settings that take units (byte size, time), we should require a unit [core] for settings that take units (byte size, time), we should require a unit Sep 7, 2014
@mikemccand mikemccand added v1.5.0 and removed v1.4.0.Beta1 labels Sep 8, 2014
@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2014

I moved this to 1.5.0 ... too dangerous to push into 1.4.0.

@s1monw s1monw added v1.6.0 and removed v1.5.0 labels Mar 17, 2015
@clintongormley clintongormley removed the v1.6.0 label May 29, 2015
mikemccand pushed a commit that referenced this issue Jun 4, 2015
Core: time-duration and byte-sized settings now require explicit units.

On upgrade, if there are any cluster or index settings that are missing units, a warning is logged and the default unit is applied.

Closes #7616 
Closes #10888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.