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 setting by time unit #37192

Merged
merged 3 commits into from
Jan 7, 2019
Merged

Conversation

jasontedor
Copy link
Member

This commit fixes an issue with a settings builder method that allows setting a duration by time unit. In particular, this method can suffer from a loss of precision. For example, if the input duration is 1500 microseconds then internally we are converting this to "1ms", demonstrating the loss of precision. Instead, we should internally convert this to a TimeValue that correctly represents the input duration, and then convert this to a string using a method that does not lose the unit. That is what this commit does.

This commit fixes an issue with a settings builder method that allows
setting a duration by time unit. In particular, this method can suffer
from a loss of precision. For example, if the input duration is 1500
microseconds then internally we are converting this to "1ms",
demonstrating the loss of precision. Instead, we should internally
convert this to a TimeValue that correctly represents the input
duration, and then convert this to a string using a method that does not
lose the unit. That is what this commit does.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member Author

This is related to #37171.

@dnhatn
Copy link
Member

dnhatn commented Jan 7, 2019

@jasontedor It seems that you forgot to include the fix.

@jasontedor
Copy link
Member Author

@dnhatn Oops, thanks. I missed it in git add.

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.

@jasontedor
Copy link
Member Author

@elasticmachine run gradle build tests 1

@jasontedor jasontedor merged commit 3b48b99 into elastic:master Jan 7, 2019
jasontedor added a commit that referenced this pull request Jan 7, 2019
This commit fixes an issue with a settings builder method that allows
setting a duration by time unit. In particular, this method can suffer
from a loss of precision. For example, if the input duration is 1500
microseconds then internally we are converting this to "1ms",
demonstrating the loss of precision. Instead, we should internally
convert this to a TimeValue that correctly represents the input
duration, and then convert this to a string using a method that does not
lose the unit. That is what this commit does.
jasontedor added a commit that referenced this pull request Jan 7, 2019
This commit fixes an issue with a settings builder method that allows
setting a duration by time unit. In particular, this method can suffer
from a loss of precision. For example, if the input duration is 1500
microseconds then internally we are converting this to "1ms",
demonstrating the loss of precision. Instead, we should internally
convert this to a TimeValue that correctly represents the input
duration, and then convert this to a string using a method that does not
lose the unit. That is what this commit does.
@jasontedor jasontedor deleted the set-by-time-unit branch January 7, 2019 23:03
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 7, 2019
…piration

* elastic/master:
  Removing unused methods in Numbers (elastic#37186)
  Fix setting by time unit (elastic#37192)
  [DOCS] Cleans up xpackml attributes
  ML: fix delayed data annotations on secured cluster (elastic#37193)
  Update version in SearchRequest and related test
  [DOCS] Adds overview and API ref for cluster voting configurations (elastic#36954)
  ML: changing JobResultsProvider.getForecastRequestStats to support > 1 index (elastic#37157)
  Separate out validation of groups of settings (elastic#34184)
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.

4 participants