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

Script: Fix value of ctx._now to be current epoch time in milliseconds #23175

Merged
merged 3 commits into from
Feb 22, 2017

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Feb 15, 2017

In update scripts, ctx._now uses the same milliseconds value used by the
rest of the system to caluclate deltas. However, that time is not
actually epoch milliseconds, as it is derived from System.nanoTime().
This change reworks the estimated time thread in ThreadPool which this
time is based on to make available both the relative time, as well as
absolute milliseconds (epoch) which may be used with calendar system. It
also renames the EstimatedTimeThread to a more apt CachedTimeThread.

closes #23169

In update scripts, `ctx._now` uses the same milliseconds value used by the
rest of the system to caluclate deltas. However, that time is not
actually epoch milliseconds, as it is derived from `System.nanoTime()`.
This change reworks the estimated time thread in ThreadPool which this
time is based on to make available both the relative time, as well as
absolute milliseconds (epoch) which may be used with calendar system. It
also renames the EstimatedTimeThread to a more apt CachedTimeThread.

closes elastic#23169
@rjernst rjernst added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v5.3.0 v5.4.0 v6.0.0-alpha1 labels Feb 15, 2017
@rjernst
Copy link
Member Author

rjernst commented Feb 15, 2017

There weren't any existing tests for the estimated time methods. I'm not sure what the best way to test them would be.

@s1monw
Copy link
Contributor

s1monw commented Feb 15, 2017

There weren't any existing tests for the estimated time methods. I'm not sure what the best way to test them would be.

@rjernst should we test that it's within a certain time range of the current time in millis is?

that would at least catch if we'd use nano time?

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM modulo adding a test like @s1monw suggested.

@rjernst rjernst merged commit 18f57c0 into elastic:master Feb 22, 2017
@rjernst rjernst deleted the script_now branch February 22, 2017 23:11
rjernst added a commit that referenced this pull request Feb 23, 2017
…nds (#23175)

In update scripts, `ctx._now` uses the same milliseconds value used by the
rest of the system to calculate deltas. However, that time is not
actually epoch milliseconds, as it is derived from `System.nanoTime()`.
This change reworks the estimated time thread in ThreadPool which this
time is based on to make available both the relative time, as well as
absolute milliseconds (epoch) which may be used with calendar system. It
also renames the EstimatedTimeThread to a more apt CachedTimeThread.

closes #23169
rjernst added a commit that referenced this pull request Feb 23, 2017
…nds (#23175)

In update scripts, `ctx._now` uses the same milliseconds value used by the
rest of the system to calculate deltas. However, that time is not
actually epoch milliseconds, as it is derived from `System.nanoTime()`.
This change reworks the estimated time thread in ThreadPool which this
time is based on to make available both the relative time, as well as
absolute milliseconds (epoch) which may be used with calendar system. It
also renames the EstimatedTimeThread to a more apt CachedTimeThread.

closes #23169
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 24, 2017
* master: (54 commits)
  Keep the pipeline handler queue small initially
  Do not create String instances in 'Strings' methods accepting StringBuilder (elastic#22907)
  Tests: fix AwsS3ServiceImplTests
  Remove abstract InternalMetricsAggregation class (elastic#23326)
  Add BulkRequest support to High Level Rest client (elastic#23312)
  Wrap getCredentials() in a doPrivileged() block (elastic#23297)
  Respect promises on pipelined responses
  Align REST specs for HEAD requests
  Remove unnecessary result sorting in SearchPhaseController (elastic#23321)
  Fix SamplerAggregatorTests to have stable and predictable docIds
  Tests: Ensure multi node integ tests wait on first node
  Relocate a comment in HttpPipeliningHandler
  Add comments to HttpPipeliningHandler
  [TEST] Fix incorrect test cluster name in cluster health doc tests
  Build: Change location in zip of license and notice inclusion for plugins (elastic#23316)
  Script: Fix value of `ctx._now` to be current epoch time in milliseconds (elastic#23175)
  Build: Rework integ test setup and shutdown to ensure stop runs when desired (elastic#23304)
  Handle long overflow when adding paths' totals
  Don't set local node on cluster state used for node join validation (elastic#23311)
  Ensure that releasing listener is called
  ...
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 14, 2018
In elastic#23175 we renamed `ThreadPool$EstimatedTimeThread` to
`ThreadPool$CachedTimeThread` but did not update the corresponding entry in
`HotThreads#isIdleThread`. This commit addresses this.
DaveCTurner added a commit that referenced this pull request Nov 15, 2018
In #23175 we renamed `ThreadPool$EstimatedTimeThread` to
`ThreadPool$CachedTimeThread` but did not update the corresponding entry in
`HotThreads#isIdleThread`. This commit addresses this.
DaveCTurner added a commit that referenced this pull request Nov 15, 2018
In #23175 we renamed `ThreadPool$EstimatedTimeThread` to
`ThreadPool$CachedTimeThread` but did not update the corresponding entry in
`HotThreads#isIdleThread`. This commit addresses this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v5.3.0 v5.4.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ctx._now time seems to be incorrect
4 participants