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 RecoveryState timestamps #11871

Closed
wants to merge 1 commit into from

Conversation

@masaruh
Copy link
Contributor

commented Jun 25, 2015

Use System.currentTimeMillis() instead of System.nanoTime() since it's for wall clock time.
Fixes #11870.

@bleskes

This comment has been minimized.

Copy link
Member

commented Jun 25, 2015

@masaruh talked a bout using System.currentTimeMillis for the time markers and nano time for the delta.

@bleskes bleskes self-assigned this Jun 25, 2015
@masaruh masaruh force-pushed the masaruh:fix/11870 branch Jun 25, 2015
@clintongormley clintongormley added the >bug label Jun 25, 2015
@@ -435,6 +439,7 @@ public synchronized void reset() {
@Override
public synchronized void readFrom(StreamInput in) throws IOException {
startTime = in.readVLong();
startNanoTime = in.readVLong();

This comment has been minimized.

Copy link
@mikemccand

mikemccand Jun 25, 2015

Contributor

If this goes back to 1.x, I think we have to conditionalize this by version?

This comment has been minimized.

Copy link
@masaruh

masaruh Jun 29, 2015

Author Contributor

Will do.

@@ -442,6 +447,7 @@ public synchronized void readFrom(StreamInput in) throws IOException {
@Override
public synchronized void writeTo(StreamOutput out) throws IOException {
out.writeVLong(startTime);
out.writeVLong(startNanoTime);

This comment has been minimized.

Copy link
@mikemccand

mikemccand Jun 25, 2015

Contributor

Same here.

This comment has been minimized.

Copy link
@masaruh

masaruh Jun 29, 2015

Author Contributor

Will do.

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

Sorry, I caused this with #11058! I didn't realize we were also using the absolute msec values against a calendar (not just for elapsed msec).

@masaruh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

So, backport would look like this? masaruh@4ecd530

@bleskes
bleskes reviewed Jun 29, 2015
View changes
core/src/test/java/org/elasticsearch/indices/recovery/RecoveryStateTest.java Outdated
@@ -154,7 +154,7 @@ public void run() {
if (randomBoolean()) {
timer.stop();
assertThat(timer.stopTime(), greaterThanOrEqualTo(timer.startTime()));
assertThat(timer.time(), equalTo(timer.stopTime() - timer.startTime()));
assertThat(timer.time(), lessThanOrEqualTo(timer.stopTime() - timer.startTime()));

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 29, 2015

Member

Are we sure we can use lessThanOrEqualTo here? we can also just assert that time() > 0 imho.

This comment has been minimized.

Copy link
@masaruh

masaruh Jun 29, 2015

Author Contributor

I think lessThanOrEqualTo is correct. (because it's rounded down to nearest decimal value)

If we use nano seconds, when start time is 1ns and stop time is 1000000ns (1ms), time() would be 99999ns but it's 0ms because of TimeValue.nsecToMSec.
But if we use milliseconds, above becomes 1ms - 0ms = 1ms. In this case, time() < stop() - start().
When start time is 1ns (0ms) and stop time is 1999999ns (1ms), it's 1999998ns but time() will be 1ms and stop() - start() = 1ms.

That's said, I like time() > 0 since it makes it simpler.

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 29, 2015

Member

I see what your are saying but I don’t think we can rely on this. The nanoTime() is not guaranteed to actually have nano precision (just resolution). it is only guaranteed to never go back…

On 29 Jun 2015, at 10:39, Masaru Hasegawa notifications@github.com wrote:

In core/src/test/java/org/elasticsearch/indices/recovery/RecoveryStateTest.java:

@@ -154,7 +154,7 @@ public void run() {
if (randomBoolean()) {
timer.stop();
assertThat(timer.stopTime(), greaterThanOrEqualTo(timer.startTime()));

  •        assertThat(timer.time(), equalTo(timer.stopTime() - timer.startTime()));
    
  •        assertThat(timer.time(), lessThanOrEqualTo(timer.stopTime() - timer.startTime()));
    

I think lessThanOrEqualTo is correct. (because it's rounded down to nearest decimal value)

If we use nano seconds, when start time is 1ns and stop time is 1000000ns (1ms), time() would be 99999ns but it's 0ms because of TimeValue.nsecToMSec.
But if we use milliseconds, above becomes 1ms - 0ms = 1ms. In this case, time() < stop() - start().
When start time is 1ns (0ms) and stop time is 1999999ns (1ms), it's 1999998ns but time() will be 1ms and stop() - start() = 1ms.

That's said, I like time() > 0 since it makes it simpler.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@masaruh

masaruh Jun 29, 2015

Author Contributor

Oh, I see. time() > 0 is definitely better then!

@bleskes

This comment has been minimized.

Copy link
Member

commented Jun 29, 2015

Thx @masaruh . Looks great - I left one question about the test and a simple comment on the backport (which looks good).

@masaruh masaruh force-pushed the masaruh:fix/11870 branch Jun 29, 2015
@masaruh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

Thanks @bleskes. Changed test to use >= 0. Since it can be 0ms when it's done really quick.

@bleskes

This comment has been minimized.

@masaruh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

Ah right. That one should have been greaterThan. But I believe another one https://github.com/elastic/elasticsearch/pull/11871/files#diff-064f839a8d24936e06aeb470ef25c9faR289 can be 0.

Use System.currentTimeMillis() for wall clock time and use System.nanoTime() for delta.
Fixes #11870.
@masaruh masaruh force-pushed the masaruh:fix/11870 branch to 4b5c6a0 Jun 29, 2015
@masaruh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

Pushed another fix.

@bleskes

This comment has been minimized.

Copy link
Member

commented Jun 29, 2015

awesome. LGTM

@masaruh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

@bleskes thanks, also pushed to 1.x and 1.6.

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.