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

Fix handling of fractional time value settings #37171

Merged

Conversation

jasontedor
Copy link
Member

This commit addresses an issue when setting a time value setting using a value that has a fractional component when converted to its string representation. For example, trying to set a time value setting to a value of 1500ms is problematic because internally this is converted to the string "1.5s". When we go to get this setting, we try to parse "1.5s" back to a time value, which does not support fractional values. The problem is that internally we are relying on a method which loses the unit when doing the string conversion. Instead, we are going to use a method that does not lose the unit and therefore we can roundtrip from the time value to the string and back to the time value.

This commit addresses an issue when setting a time value setting using a
value that has a fractional component when converted to its string
representation. For example, trying to set a time value setting to a
value of 1500ms is problematic because internally this is converted to
the string "1.5s". When we go to get this setting, we try to parse
"1.5s" back to a time value, which does not support fractional
values. The problem is that internally we are relying on a method which
loses the unit when doing the string conversion. Instead, we are going
to use a method that does not lose the unit and therefore we can
roundtrip from the time value to the string and back to the time value.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member Author

This was discovered in the course of adding expiration on top of #37167 in preparation for #37165. A similar issue is existing for byte size values which I will address in another pull request.

@jasontedor
Copy link
Member Author

A similar issue is existing for byte size values which I will address in another pull request.

For byte size value settings I have opened #37172.

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Nice find.
Should we also fix Setting#put(String setting, long value, TimeUnit timeUnit) which possibly round the input? It is not related to your PR - maybe a follow-up?

@jasontedor jasontedor merged commit bf5bc88 into elastic:master Jan 7, 2019
jasontedor added a commit that referenced this pull request Jan 7, 2019
This commit addresses an issue when setting a time value setting using a
value that has a fractional component when converted to its string
representation. For example, trying to set a time value setting to a
value of 1500ms is problematic because internally this is converted to
the string "1.5s". When we go to get this setting, we try to parse
"1.5s" back to a time value, which does not support fractional
values. The problem is that internally we are relying on a method which
loses the unit when doing the string conversion. Instead, we are going
to use a method that does not lose the unit and therefore we can
roundtrip from the time value to the string and back to the time value.
jasontedor added a commit that referenced this pull request Jan 7, 2019
This commit addresses an issue when setting a time value setting using a
value that has a fractional component when converted to its string
representation. For example, trying to set a time value setting to a
value of 1500ms is problematic because internally this is converted to
the string "1.5s". When we go to get this setting, we try to parse
"1.5s" back to a time value, which does not support fractional
values. The problem is that internally we are relying on a method which
loses the unit when doing the string conversion. Instead, we are going
to use a method that does not lose the unit and therefore we can
roundtrip from the time value to the string and back to the time value.
@jasontedor jasontedor deleted the fractional-time-value-settings branch January 7, 2019 06:48
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 7, 2019
* master:
  Fix handling of fractional time value settings (elastic#37171)
@jasontedor
Copy link
Member Author

Should we also fix Setting#put(String setting, long value, TimeUnit timeUnit) which possibly round the input? It is not related to your PR - maybe a follow-up?

Thanks @dnhatn. I opened #37192.

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.

None yet

4 participants