Skip to content

Commit

Permalink
Fix Bug in Snapshot Status Response Timestamps (#46919) (#46970)
Browse files Browse the repository at this point in the history
Fixing a corner case where snapshot total time calculation was off when
getting the `SnapshotStatus` of an in-progress snapshot.

Closes #46913
  • Loading branch information
original-brownbear committed Sep 23, 2019
1 parent f06aa0c commit 2da0406
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action.admin.cluster.snapshots.status;

import org.elasticsearch.Version;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
Expand Down Expand Up @@ -70,6 +71,7 @@ public class SnapshotStats implements Writeable, ToXContentObject {
long incrementalSize, long totalSize, long processedSize) {
this.startTime = startTime;
this.time = time;
assert time >= 0 : "Tried to initialize snapshot stats with negative total time [" + time + "]";
this.incrementalFileCount = incrementalFileCount;
this.totalFileCount = totalFileCount;
this.processedFileCount = processedFileCount;
Expand Down Expand Up @@ -323,6 +325,8 @@ void add(SnapshotStats stats, boolean updateTimestamps) {
// Update duration
time = endTime - startTime;
}
assert time >= 0
: "Update with [" + Strings.toString(stats) + "][" + updateTimestamps + "] resulted in negative total time [" + time + "]";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public class SnapshotStatus implements ToXContentObject, Writeable {
this.shards = Objects.requireNonNull(shards);
this.includeGlobalState = includeGlobalState;
shardsStats = new SnapshotShardsStats(shards);
assert time >= 0 : "time must be >= 0 but received [" + time + "]";
updateShardStats(startTime, time);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,14 @@ private SnapshotsStatusResponse buildResponse(SnapshotsStatusRequest request, Li
throw new IllegalArgumentException("Unknown snapshot state " + snapshotInfo.state());
}
final long startTime = snapshotInfo.startTime();
final long endTime = snapshotInfo.endTime();
assert endTime >= startTime || (endTime == 0L && snapshotInfo.state().completed() == false)
: "Inconsistent timestamps found in SnapshotInfo [" + snapshotInfo + "]";
builder.add(new SnapshotStatus(new Snapshot(repositoryName, snapshotId), state,
Collections.unmodifiableList(shardStatusBuilder), snapshotInfo.includeGlobalState(),
startTime, snapshotInfo.endTime() - startTime));
startTime,
// Use current time to calculate overall runtime for in-progress snapshots that have endTime == 0
(endTime == 0 ? threadPool.absoluteTimeInMillis() : endTime) - startTime));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ public class SnapshotStatsTests extends AbstractXContentTestCase<SnapshotStats>

@Override
protected SnapshotStats createTestInstance() {
long startTime = randomNonNegativeLong();
long time = randomNonNegativeLong();
// Using less than half of Long.MAX_VALUE for random time values to avoid long overflow in tests that add the two time values
long startTime = randomLongBetween(0, Long.MAX_VALUE / 2 - 1);
long time = randomLongBetween(0, Long.MAX_VALUE / 2 - 1);
int incrementalFileCount = randomIntBetween(0, Integer.MAX_VALUE);
int totalFileCount = randomIntBetween(0, Integer.MAX_VALUE);
int processedFileCount = randomIntBetween(0, Integer.MAX_VALUE);
Expand Down

0 comments on commit 2da0406

Please sign in to comment.