Skip to content

Fixes ByteSizeValue to serialise correctly #27702

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 14, 2017
Merged

Fixes ByteSizeValue to serialise correctly #27702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c4d615b
Fixes ByteSizeValue to serialise correctly
colings86 Dec 6, 2017
9d23322
Fix bytes settings default value to not use fractional values
colings86 Dec 7, 2017
960cfa1
Fixes test
colings86 Dec 7, 2017
f8792f6
Addresses review comments
colings86 Dec 12, 2017
677c869
Modifies parsing to preserve unit
colings86 Dec 12, 2017
b6501da
Addresses more review comments
colings86 Dec 13, 2017
f49815f
Fixes tests
colings86 Dec 13, 2017
6753cd3
Temporarily changes version check to 7.0.0
colings86 Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/src/main/java/org/elasticsearch/common/settings/Setting.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import java.util.function.BiConsumer;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.IntConsumer;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -1020,7 +1019,7 @@ public static Setting<ByteSizeValue> byteSizeSetting(String key, Function<Settin

public static Setting<ByteSizeValue> byteSizeSetting(String key, ByteSizeValue defaultValue, ByteSizeValue minValue,
ByteSizeValue maxValue, Property... properties) {
return byteSizeSetting(key, (s) -> defaultValue.toString(), minValue, maxValue, properties);
return byteSizeSetting(key, (s) -> defaultValue.getStringRep(), minValue, maxValue, properties);
}

public static Setting<ByteSizeValue> byteSizeSetting(String key, Function<Settings, String> defaultValue,
Expand Down
32 changes: 32 additions & 0 deletions core/src/main/java/org/elasticsearch/common/unit/ByteSizeUnit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ public long toTB(long size) {
public long toPB(long size) {
return size / (C5 / C0);
}

@Override
public String getSuffix() {
return "b";
}
},
KB {
@Override
Expand Down Expand Up @@ -94,6 +99,11 @@ public long toTB(long size) {
public long toPB(long size) {
return size / (C5 / C1);
}

@Override
public String getSuffix() {
return "kb";
}
},
MB {
@Override
Expand Down Expand Up @@ -125,6 +135,11 @@ public long toTB(long size) {
public long toPB(long size) {
return size / (C5 / C2);
}

@Override
public String getSuffix() {
return "mb";
}
},
GB {
@Override
Expand Down Expand Up @@ -156,6 +171,11 @@ public long toTB(long size) {
public long toPB(long size) {
return size / (C5 / C3);
}

@Override
public String getSuffix() {
return "gb";
}
},
TB {
@Override
Expand Down Expand Up @@ -187,6 +207,11 @@ public long toTB(long size) {
public long toPB(long size) {
return size / (C5 / C4);
}

@Override
public String getSuffix() {
return "tb";
}
},
PB {
@Override
Expand Down Expand Up @@ -218,6 +243,11 @@ public long toTB(long size) {
public long toPB(long size) {
return size;
}

@Override
public String getSuffix() {
return "pb";
}
};

static final long C0 = 1L;
Expand Down Expand Up @@ -258,6 +288,8 @@ static long x(long d, long m, long over) {

public abstract long toPB(long size);

public abstract String getSuffix();

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeVInt(this.ordinal());
Expand Down
166 changes: 115 additions & 51 deletions core/src/main/java/org/elasticsearch/common/unit/ByteSizeValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,39 +20,71 @@
package org.elasticsearch.common.unit;

import org.elasticsearch.ElasticsearchParseException;
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;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.Loggers;

import java.io.IOException;
import java.util.Locale;
import java.util.Objects;

public class ByteSizeValue implements Writeable, Comparable<ByteSizeValue> {
private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ByteSizeValue.class));

private final long size;
private final ByteSizeUnit unit;

public ByteSizeValue(StreamInput in) throws IOException {
size = in.readVLong();
unit = ByteSizeUnit.BYTES;
if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
size = in.readVLong();
unit = ByteSizeUnit.BYTES;
} else {
size = in.readZLong();
unit = ByteSizeUnit.readFrom(in);
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeVLong(getBytes());
if (out.getVersion().before(Version.V_7_0_0_alpha1)) {
out.writeVLong(getBytes());
} else {
out.writeZLong(size);
unit.writeTo(out);
}
}

public ByteSizeValue(long bytes) {
this(bytes, ByteSizeUnit.BYTES);
}

public ByteSizeValue(long size, ByteSizeUnit unit) {
if (size < -1 || (size == -1 && unit != ByteSizeUnit.BYTES)) {
throw new IllegalArgumentException("Values less than -1 bytes are not supported: " + size + unit.getSuffix());
}
if (size > Long.MAX_VALUE / unit.toBytes(1)) {
throw new IllegalArgumentException(
"Values greater than " + Long.MAX_VALUE + " bytes are not supported: " + size + unit.getSuffix());
}
this.size = size;
this.unit = unit;
}

// For testing
long getSize() {
return size;
}

// For testing
ByteSizeUnit getUnit() {
return unit;
}

@Deprecated
public int bytesAsInt() {
long bytes = getBytes();
if (bytes > Integer.MAX_VALUE) {
Expand Down Expand Up @@ -105,26 +137,41 @@ public double getPbFrac() {
return ((double) getBytes()) / ByteSizeUnit.C5;
}

/**
* @return a string representation of this value which is guaranteed to be
* able to be parsed using
* {@link #parseBytesSizeValue(String, ByteSizeValue, String)}.
* Unlike {@link #toString()} this method will not output fractional
* or rounded values so this method should be preferred when
* serialising the value to JSON.
*/
public String getStringRep() {
if (size <= 0) {
return String.valueOf(size);
}
return size + unit.getSuffix();
}

@Override
public String toString() {
long bytes = getBytes();
double value = bytes;
String suffix = "b";
String suffix = ByteSizeUnit.BYTES.getSuffix();
if (bytes >= ByteSizeUnit.C5) {
value = getPbFrac();
suffix = "pb";
suffix = ByteSizeUnit.PB.getSuffix();
} else if (bytes >= ByteSizeUnit.C4) {
value = getTbFrac();
suffix = "tb";
suffix = ByteSizeUnit.TB.getSuffix();
} else if (bytes >= ByteSizeUnit.C3) {
value = getGbFrac();
suffix = "gb";
suffix = ByteSizeUnit.GB.getSuffix();
} else if (bytes >= ByteSizeUnit.C2) {
value = getMbFrac();
suffix = "mb";
suffix = ByteSizeUnit.MB.getSuffix();
} else if (bytes >= ByteSizeUnit.C1) {
value = getKbFrac();
suffix = "kb";
suffix = ByteSizeUnit.KB.getSuffix();
}
return Strings.format1Decimals(value, suffix);
}
Expand All @@ -139,47 +186,64 @@ public static ByteSizeValue parseBytesSizeValue(String sValue, ByteSizeValue def
if (sValue == null) {
return defaultValue;
}
long bytes;
String lowerSValue = sValue.toLowerCase(Locale.ROOT).trim();
if (lowerSValue.endsWith("k")) {
return parse(sValue, lowerSValue, "k", ByteSizeUnit.KB, settingName);
} else if (lowerSValue.endsWith("kb")) {
return parse(sValue, lowerSValue, "kb", ByteSizeUnit.KB, settingName);
} else if (lowerSValue.endsWith("m")) {
return parse(sValue, lowerSValue, "m", ByteSizeUnit.MB, settingName);
} else if (lowerSValue.endsWith("mb")) {
return parse(sValue, lowerSValue, "mb", ByteSizeUnit.MB, settingName);
} else if (lowerSValue.endsWith("g")) {
return parse(sValue, lowerSValue, "g", ByteSizeUnit.GB, settingName);
} else if (lowerSValue.endsWith("gb")) {
return parse(sValue, lowerSValue, "gb", ByteSizeUnit.GB, settingName);
} else if (lowerSValue.endsWith("t")) {
return parse(sValue, lowerSValue, "t", ByteSizeUnit.TB, settingName);
} else if (lowerSValue.endsWith("tb")) {
return parse(sValue, lowerSValue, "tb", ByteSizeUnit.TB, settingName);
} else if (lowerSValue.endsWith("p")) {
return parse(sValue, lowerSValue, "p", ByteSizeUnit.PB, settingName);
} else if (lowerSValue.endsWith("pb")) {
return parse(sValue, lowerSValue, "pb", ByteSizeUnit.PB, settingName);
} else if (lowerSValue.endsWith("b")) {
return new ByteSizeValue(Long.parseLong(lowerSValue.substring(0, lowerSValue.length() - 1).trim()), ByteSizeUnit.BYTES);
} else if (lowerSValue.equals("-1")) {
// Allow this special value to be unit-less:
return new ByteSizeValue(-1, ByteSizeUnit.BYTES);
} else if (lowerSValue.equals("0")) {
// Allow this special value to be unit-less:
return new ByteSizeValue(0, ByteSizeUnit.BYTES);
} else {
// Missing units:
throw new ElasticsearchParseException(
"failed to parse setting [{}] with value [{}] as a size in bytes: unit is missing or unrecognized", settingName,
sValue);
}
}

private static ByteSizeValue parse(final String initialInput, final String normalized, final String suffix, ByteSizeUnit unit,
final String settingName) {
final String s = normalized.substring(0, normalized.length() - suffix.length()).trim();
try {
String lowerSValue = sValue.toLowerCase(Locale.ROOT).trim();
if (lowerSValue.endsWith("k")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C1);
} else if (lowerSValue.endsWith("kb")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C1);
} else if (lowerSValue.endsWith("m")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C2);
} else if (lowerSValue.endsWith("mb")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C2);
} else if (lowerSValue.endsWith("g")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C3);
} else if (lowerSValue.endsWith("gb")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C3);
} else if (lowerSValue.endsWith("t")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C4);
} else if (lowerSValue.endsWith("tb")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C4);
} else if (lowerSValue.endsWith("p")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C5);
} else if (lowerSValue.endsWith("pb")) {
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C5);
} else if (lowerSValue.endsWith("b")) {
bytes = Long.parseLong(lowerSValue.substring(0, lowerSValue.length() - 1).trim());
} else if (lowerSValue.equals("-1")) {
// Allow this special value to be unit-less:
bytes = -1;
} else if (lowerSValue.equals("0")) {
// Allow this special value to be unit-less:
bytes = 0;
} else {
// Missing units:
throw new ElasticsearchParseException(
"failed to parse setting [{}] with value [{}] as a size in bytes: unit is missing or unrecognized",
settingName, sValue);
try {
return new ByteSizeValue(Long.parseLong(s), unit);
} catch (final NumberFormatException e) {
try {
final double doubleValue = Double.parseDouble(s);
DEPRECATION_LOGGER.deprecated(
"Fractional bytes values are deprecated. Use non-fractional bytes values instead: [{}] found for setting [{}]",
initialInput, settingName);
return new ByteSizeValue((long) (doubleValue * unit.toBytes(1)));
} catch (final NumberFormatException ignored) {
throw new ElasticsearchParseException("failed to parse [{}]", e, initialInput);
}
}
} catch (NumberFormatException e) {
throw new ElasticsearchParseException("failed to parse [{}]", e, sValue);
} catch (IllegalArgumentException e) {
throw new ElasticsearchParseException("failed to parse setting [{}] with value [{}] as a size in bytes", e, settingName,
initialInput);
}
return new ByteSizeValue(bytes, ByteSizeUnit.BYTES);
}

@Override
Expand All @@ -196,13 +260,13 @@ public boolean equals(Object o) {

@Override
public int hashCode() {
return Double.hashCode(((double) size) * unit.toBytes(1));
return Long.hashCode(size * unit.toBytes(1));
}

@Override
public int compareTo(ByteSizeValue other) {
double thisValue = ((double) size) * unit.toBytes(1);
double otherValue = ((double) other.size) * other.unit.toBytes(1);
return Double.compare(thisValue, otherValue);
long thisValue = size * unit.toBytes(1);
long otherValue = other.size * other.unit.toBytes(1);
return Long.compare(thisValue, otherValue);
}
}
6 changes: 4 additions & 2 deletions core/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStatsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,9 +311,11 @@ private static NodeStats createNodeStats() {
for (int i = 0; i < 3; i++) {
loadAverages[i] = randomBoolean() ? randomDouble() : -1;
}
long memTotal = randomNonNegativeLong();
long swapTotal = randomNonNegativeLong();
osStats = new OsStats(System.currentTimeMillis(), new OsStats.Cpu(randomShort(), loadAverages),
new OsStats.Mem(randomLong(), randomLong()),
new OsStats.Swap(randomLong(), randomLong()),
new OsStats.Mem(memTotal, randomLongBetween(0, memTotal)),
new OsStats.Swap(swapTotal, randomLongBetween(0, swapTotal)),
new OsStats.Cgroup(
randomAlphaOfLength(8),
randomNonNegativeLong(),
Expand Down
7 changes: 2 additions & 5 deletions core/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,12 @@
import org.elasticsearch.action.admin.cluster.node.stats.NodeStats;
import org.elasticsearch.action.admin.indices.stats.CommonStats;
import org.elasticsearch.action.admin.indices.stats.ShardStats;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.routing.RecoverySource.PeerRecoverySource;
import org.elasticsearch.cluster.routing.ShardRouting;
import org.elasticsearch.cluster.routing.ShardRoutingHelper;
import org.elasticsearch.cluster.routing.UnassignedInfo;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.index.shard.ShardPath;
Expand Down Expand Up @@ -184,12 +181,12 @@ public void testFillDiskUsageSomeInvalidValues() {
new FsInfo.Path("/most", "/dev/sdc", 300, 290, 280),
};
FsInfo.Path[] node2FSInfo = new FsInfo.Path[] {
new FsInfo.Path("/least_most", "/dev/sda", -2, -1, -1),
new FsInfo.Path("/least_most", "/dev/sda", -1, -1, -1),
};

FsInfo.Path[] node3FSInfo = new FsInfo.Path[] {
new FsInfo.Path("/most", "/dev/sda", 100, 90, 70),
new FsInfo.Path("/least", "/dev/sda", 10, -8, 0),
new FsInfo.Path("/least", "/dev/sda", 10, -1, 0),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasontedor this is the test I am unsure about. It failed when I added code to reject negative bytes values (expect the special -1 value) because this test explicitly checked that negative values for disk stats were accepted but the disk is skipped. From looking at FSProbe#L143 which is where we gather the disk statistics it seems that we can't actually get negative values here and instead any value reported by the OS as negative are interpreted by FSProbe as an overflow and are converted to Long.MAX_VALUE.

I changed this test to use the special -1 value instead and check the disks are still skipped but maybe we actually don't need this test at all?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, the test was doing unrealistic things so I am fine with the change you made here. Let's keep whether or not the test is needed as a separate issue. There's probably other cleanup to do here, like adding asserts in FsInfo.Path about not seeing negative values.

};
List<NodeStats> nodeStats = Arrays.asList(
new NodeStats(new DiscoveryNode("node_1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0,
Expand Down
Loading