Skip to content

Commit c355677

Browse files
colings86colings86
committed
Fixes ByteSizeValue to serialise correctly (#27702)
* Fixes ByteSizeValue to serialise correctly This fix makes a few fixes to ByteSizeValue to make it possible to perform round-trip serialisation: * Changes wire serialisation to use Zlong methods instead of VLong methods. This is needed because the value `-1` is accepted but previously if `-1` is supplied it cannot be serialised using the wire protocol. * Limits the supplied size to be no more than Long.MAX_VALUE when converted to bytes. Previously values greater than Long.MAX_VALUE bytes were accepted but would be silently interpreted as Long.MAX_VALUE bytes rather than erroring so the user had no idea the value was not being used the way they had intended. I consider this a bug and so fine to include this bug fix in a minor version but I am open to other points of view. * Adds a `getStringRep()` method that can be used when serialising the value to JSON. This will print the bytes value if the size is positive, `”0”` if the size is `0` and `”-1”` if the size is `-1`. * Adds logic to detect fractional values when parsing from a String and emits a deprecation warning in this case. * Modifies hashCode and equals methods to work with long values rather than doubles so they don’t run into precision problems when dealing with large values. Previous to this change the equals method would not detect small differences in the values (e.g. 1-1000 bytes ranges) if the actual values where very large (e.g. PBs). This was due to the values being in the order of 10^18 but doubles only maintaining a precision of ~10^15. Closes #27568 * Fix bytes settings default value to not use fractional values * Fixes test * Addresses review comments * Modifies parsing to preserve unit This should be bwc since in the case that the input is fractional it reverts back to the old method of parsing it to the bytes value. * Addresses more review comments * Fixes tests * Temporarily changes version check to 7.0.0 This will be changed to 6.2 when the fix has been backported
1 parent 7ec8f7d commit c355677

File tree

10 files changed

+314
-100
lines changed

10 files changed

+314
-100
lines changed

‎core/src/main/java/org/elasticsearch/common/settings/Setting.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
import java.util.function.BiConsumer;
5353
import java.util.function.Consumer;
5454
import java.util.function.Function;
55-
import java.util.function.IntConsumer;
5655
import java.util.regex.Matcher;
5756
import java.util.regex.Pattern;
5857
import java.util.stream.Collectors;
@@ -1020,7 +1019,7 @@ public static Setting<ByteSizeValue> byteSizeSetting(String key, Function<Settin
10201019

10211020
public static Setting<ByteSizeValue> byteSizeSetting(String key, ByteSizeValue defaultValue, ByteSizeValue minValue,
10221021
ByteSizeValue maxValue, Property... properties) {
1023-
return byteSizeSetting(key, (s) -> defaultValue.toString(), minValue, maxValue, properties);
1022+
return byteSizeSetting(key, (s) -> defaultValue.getStringRep(), minValue, maxValue, properties);
10241023
}
10251024

10261025
public static Setting<ByteSizeValue> byteSizeSetting(String key, Function<Settings, String> defaultValue,

‎core/src/main/java/org/elasticsearch/common/unit/ByteSizeUnit.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ public long toTB(long size) {
6363
public long toPB(long size) {
6464
return size / (C5 / C0);
6565
}
66+
67+
@Override
68+
public String getSuffix() {
69+
return "b";
70+
}
6671
},
6772
KB {
6873
@Override
@@ -94,6 +99,11 @@ public long toTB(long size) {
9499
public long toPB(long size) {
95100
return size / (C5 / C1);
96101
}
102+
103+
@Override
104+
public String getSuffix() {
105+
return "kb";
106+
}
97107
},
98108
MB {
99109
@Override
@@ -125,6 +135,11 @@ public long toTB(long size) {
125135
public long toPB(long size) {
126136
return size / (C5 / C2);
127137
}
138+
139+
@Override
140+
public String getSuffix() {
141+
return "mb";
142+
}
128143
},
129144
GB {
130145
@Override
@@ -156,6 +171,11 @@ public long toTB(long size) {
156171
public long toPB(long size) {
157172
return size / (C5 / C3);
158173
}
174+
175+
@Override
176+
public String getSuffix() {
177+
return "gb";
178+
}
159179
},
160180
TB {
161181
@Override
@@ -187,6 +207,11 @@ public long toTB(long size) {
187207
public long toPB(long size) {
188208
return size / (C5 / C4);
189209
}
210+
211+
@Override
212+
public String getSuffix() {
213+
return "tb";
214+
}
190215
},
191216
PB {
192217
@Override
@@ -218,6 +243,11 @@ public long toTB(long size) {
218243
public long toPB(long size) {
219244
return size;
220245
}
246+
247+
@Override
248+
public String getSuffix() {
249+
return "pb";
250+
}
221251
};
222252

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

259289
public abstract long toPB(long size);
260290

291+
public abstract String getSuffix();
292+
261293
@Override
262294
public void writeTo(StreamOutput out) throws IOException {
263295
out.writeVInt(this.ordinal());

‎core/src/main/java/org/elasticsearch/common/unit/ByteSizeValue.java‎

Lines changed: 115 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -20,39 +20,71 @@
2020
package org.elasticsearch.common.unit;
2121

2222
import org.elasticsearch.ElasticsearchParseException;
23+
import org.elasticsearch.Version;
2324
import org.elasticsearch.common.Strings;
2425
import org.elasticsearch.common.io.stream.StreamInput;
2526
import org.elasticsearch.common.io.stream.StreamOutput;
2627
import org.elasticsearch.common.io.stream.Writeable;
28+
import org.elasticsearch.common.logging.DeprecationLogger;
29+
import org.elasticsearch.common.logging.Loggers;
2730

2831
import java.io.IOException;
2932
import java.util.Locale;
3033
import java.util.Objects;
3134

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

3438
private final long size;
3539
private final ByteSizeUnit unit;
3640

3741
public ByteSizeValue(StreamInput in) throws IOException {
38-
size = in.readVLong();
39-
unit = ByteSizeUnit.BYTES;
42+
if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
43+
size = in.readVLong();
44+
unit = ByteSizeUnit.BYTES;
45+
} else {
46+
size = in.readZLong();
47+
unit = ByteSizeUnit.readFrom(in);
48+
}
4049
}
4150

4251
@Override
4352
public void writeTo(StreamOutput out) throws IOException {
44-
out.writeVLong(getBytes());
53+
if (out.getVersion().before(Version.V_7_0_0_alpha1)) {
54+
out.writeVLong(getBytes());
55+
} else {
56+
out.writeZLong(size);
57+
unit.writeTo(out);
58+
}
4559
}
4660

4761
public ByteSizeValue(long bytes) {
4862
this(bytes, ByteSizeUnit.BYTES);
4963
}
5064

5165
public ByteSizeValue(long size, ByteSizeUnit unit) {
66+
if (size < -1 || (size == -1 && unit != ByteSizeUnit.BYTES)) {
67+
throw new IllegalArgumentException("Values less than -1 bytes are not supported: " + size + unit.getSuffix());
68+
}
69+
if (size > Long.MAX_VALUE / unit.toBytes(1)) {
70+
throw new IllegalArgumentException(
71+
"Values greater than " + Long.MAX_VALUE + " bytes are not supported: " + size + unit.getSuffix());
72+
}
5273
this.size = size;
5374
this.unit = unit;
5475
}
5576

77+
// For testing
78+
long getSize() {
79+
return size;
80+
}
81+
82+
// For testing
83+
ByteSizeUnit getUnit() {
84+
return unit;
85+
}
86+
87+
@Deprecated
5688
public int bytesAsInt() {
5789
long bytes = getBytes();
5890
if (bytes > Integer.MAX_VALUE) {
@@ -105,26 +137,41 @@ public double getPbFrac() {
105137
return ((double) getBytes()) / ByteSizeUnit.C5;
106138
}
107139

140+
/**
141+
* @return a string representation of this value which is guaranteed to be
142+
* able to be parsed using
143+
* {@link #parseBytesSizeValue(String, ByteSizeValue, String)}.
144+
* Unlike {@link #toString()} this method will not output fractional
145+
* or rounded values so this method should be preferred when
146+
* serialising the value to JSON.
147+
*/
148+
public String getStringRep() {
149+
if (size <= 0) {
150+
return String.valueOf(size);
151+
}
152+
return size + unit.getSuffix();
153+
}
154+
108155
@Override
109156
public String toString() {
110157
long bytes = getBytes();
111158
double value = bytes;
112-
String suffix = "b";
159+
String suffix = ByteSizeUnit.BYTES.getSuffix();
113160
if (bytes >= ByteSizeUnit.C5) {
114161
value = getPbFrac();
115-
suffix = "pb";
162+
suffix = ByteSizeUnit.PB.getSuffix();
116163
} else if (bytes >= ByteSizeUnit.C4) {
117164
value = getTbFrac();
118-
suffix = "tb";
165+
suffix = ByteSizeUnit.TB.getSuffix();
119166
} else if (bytes >= ByteSizeUnit.C3) {
120167
value = getGbFrac();
121-
suffix = "gb";
168+
suffix = ByteSizeUnit.GB.getSuffix();
122169
} else if (bytes >= ByteSizeUnit.C2) {
123170
value = getMbFrac();
124-
suffix = "mb";
171+
suffix = ByteSizeUnit.MB.getSuffix();
125172
} else if (bytes >= ByteSizeUnit.C1) {
126173
value = getKbFrac();
127-
suffix = "kb";
174+
suffix = ByteSizeUnit.KB.getSuffix();
128175
}
129176
return Strings.format1Decimals(value, suffix);
130177
}
@@ -139,47 +186,64 @@ public static ByteSizeValue parseBytesSizeValue(String sValue, ByteSizeValue def
139186
if (sValue == null) {
140187
return defaultValue;
141188
}
142-
long bytes;
189+
String lowerSValue = sValue.toLowerCase(Locale.ROOT).trim();
190+
if (lowerSValue.endsWith("k")) {
191+
return parse(sValue, lowerSValue, "k", ByteSizeUnit.KB, settingName);
192+
} else if (lowerSValue.endsWith("kb")) {
193+
return parse(sValue, lowerSValue, "kb", ByteSizeUnit.KB, settingName);
194+
} else if (lowerSValue.endsWith("m")) {
195+
return parse(sValue, lowerSValue, "m", ByteSizeUnit.MB, settingName);
196+
} else if (lowerSValue.endsWith("mb")) {
197+
return parse(sValue, lowerSValue, "mb", ByteSizeUnit.MB, settingName);
198+
} else if (lowerSValue.endsWith("g")) {
199+
return parse(sValue, lowerSValue, "g", ByteSizeUnit.GB, settingName);
200+
} else if (lowerSValue.endsWith("gb")) {
201+
return parse(sValue, lowerSValue, "gb", ByteSizeUnit.GB, settingName);
202+
} else if (lowerSValue.endsWith("t")) {
203+
return parse(sValue, lowerSValue, "t", ByteSizeUnit.TB, settingName);
204+
} else if (lowerSValue.endsWith("tb")) {
205+
return parse(sValue, lowerSValue, "tb", ByteSizeUnit.TB, settingName);
206+
} else if (lowerSValue.endsWith("p")) {
207+
return parse(sValue, lowerSValue, "p", ByteSizeUnit.PB, settingName);
208+
} else if (lowerSValue.endsWith("pb")) {
209+
return parse(sValue, lowerSValue, "pb", ByteSizeUnit.PB, settingName);
210+
} else if (lowerSValue.endsWith("b")) {
211+
return new ByteSizeValue(Long.parseLong(lowerSValue.substring(0, lowerSValue.length() - 1).trim()), ByteSizeUnit.BYTES);
212+
} else if (lowerSValue.equals("-1")) {
213+
// Allow this special value to be unit-less:
214+
return new ByteSizeValue(-1, ByteSizeUnit.BYTES);
215+
} else if (lowerSValue.equals("0")) {
216+
// Allow this special value to be unit-less:
217+
return new ByteSizeValue(0, ByteSizeUnit.BYTES);
218+
} else {
219+
// Missing units:
220+
throw new ElasticsearchParseException(
221+
"failed to parse setting [{}] with value [{}] as a size in bytes: unit is missing or unrecognized", settingName,
222+
sValue);
223+
}
224+
}
225+
226+
private static ByteSizeValue parse(final String initialInput, final String normalized, final String suffix, ByteSizeUnit unit,
227+
final String settingName) {
228+
final String s = normalized.substring(0, normalized.length() - suffix.length()).trim();
143229
try {
144-
String lowerSValue = sValue.toLowerCase(Locale.ROOT).trim();
145-
if (lowerSValue.endsWith("k")) {
146-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C1);
147-
} else if (lowerSValue.endsWith("kb")) {
148-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C1);
149-
} else if (lowerSValue.endsWith("m")) {
150-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C2);
151-
} else if (lowerSValue.endsWith("mb")) {
152-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C2);
153-
} else if (lowerSValue.endsWith("g")) {
154-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C3);
155-
} else if (lowerSValue.endsWith("gb")) {
156-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C3);
157-
} else if (lowerSValue.endsWith("t")) {
158-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C4);
159-
} else if (lowerSValue.endsWith("tb")) {
160-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C4);
161-
} else if (lowerSValue.endsWith("p")) {
162-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 1)) * ByteSizeUnit.C5);
163-
} else if (lowerSValue.endsWith("pb")) {
164-
bytes = (long) (Double.parseDouble(lowerSValue.substring(0, lowerSValue.length() - 2)) * ByteSizeUnit.C5);
165-
} else if (lowerSValue.endsWith("b")) {
166-
bytes = Long.parseLong(lowerSValue.substring(0, lowerSValue.length() - 1).trim());
167-
} else if (lowerSValue.equals("-1")) {
168-
// Allow this special value to be unit-less:
169-
bytes = -1;
170-
} else if (lowerSValue.equals("0")) {
171-
// Allow this special value to be unit-less:
172-
bytes = 0;
173-
} else {
174-
// Missing units:
175-
throw new ElasticsearchParseException(
176-
"failed to parse setting [{}] with value [{}] as a size in bytes: unit is missing or unrecognized",
177-
settingName, sValue);
230+
try {
231+
return new ByteSizeValue(Long.parseLong(s), unit);
232+
} catch (final NumberFormatException e) {
233+
try {
234+
final double doubleValue = Double.parseDouble(s);
235+
DEPRECATION_LOGGER.deprecated(
236+
"Fractional bytes values are deprecated. Use non-fractional bytes values instead: [{}] found for setting [{}]",
237+
initialInput, settingName);
238+
return new ByteSizeValue((long) (doubleValue * unit.toBytes(1)));
239+
} catch (final NumberFormatException ignored) {
240+
throw new ElasticsearchParseException("failed to parse [{}]", e, initialInput);
241+
}
178242
}
179-
} catch (NumberFormatException e) {
180-
throw new ElasticsearchParseException("failed to parse [{}]", e, sValue);
243+
} catch (IllegalArgumentException e) {
244+
throw new ElasticsearchParseException("failed to parse setting [{}] with value [{}] as a size in bytes", e, settingName,
245+
initialInput);
181246
}
182-
return new ByteSizeValue(bytes, ByteSizeUnit.BYTES);
183247
}
184248

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

197261
@Override
198262
public int hashCode() {
199-
return Double.hashCode(((double) size) * unit.toBytes(1));
263+
return Long.hashCode(size * unit.toBytes(1));
200264
}
201265

202266
@Override
203267
public int compareTo(ByteSizeValue other) {
204-
double thisValue = ((double) size) * unit.toBytes(1);
205-
double otherValue = ((double) other.size) * other.unit.toBytes(1);
206-
return Double.compare(thisValue, otherValue);
268+
long thisValue = size * unit.toBytes(1);
269+
long otherValue = other.size * other.unit.toBytes(1);
270+
return Long.compare(thisValue, otherValue);
207271
}
208272
}

‎core/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStatsTests.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -311,9 +311,11 @@ private static NodeStats createNodeStats() {
311311
for (int i = 0; i < 3; i++) {
312312
loadAverages[i] = randomBoolean() ? randomDouble() : -1;
313313
}
314+
long memTotal = randomNonNegativeLong();
315+
long swapTotal = randomNonNegativeLong();
314316
osStats = new OsStats(System.currentTimeMillis(), new OsStats.Cpu(randomShort(), loadAverages),
315-
new OsStats.Mem(randomLong(), randomLong()),
316-
new OsStats.Swap(randomLong(), randomLong()),
317+
new OsStats.Mem(memTotal, randomLongBetween(0, memTotal)),
318+
new OsStats.Swap(swapTotal, randomLongBetween(0, swapTotal)),
317319
new OsStats.Cgroup(
318320
randomAlphaOfLength(8),
319321
randomNonNegativeLong(),

‎core/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,12 @@
2323
import org.elasticsearch.action.admin.cluster.node.stats.NodeStats;
2424
import org.elasticsearch.action.admin.indices.stats.CommonStats;
2525
import org.elasticsearch.action.admin.indices.stats.ShardStats;
26-
import org.elasticsearch.cluster.metadata.IndexMetaData;
27-
import org.elasticsearch.cluster.metadata.MetaData;
2826
import org.elasticsearch.cluster.node.DiscoveryNode;
2927
import org.elasticsearch.cluster.routing.RecoverySource.PeerRecoverySource;
3028
import org.elasticsearch.cluster.routing.ShardRouting;
3129
import org.elasticsearch.cluster.routing.ShardRoutingHelper;
3230
import org.elasticsearch.cluster.routing.UnassignedInfo;
3331
import org.elasticsearch.common.collect.ImmutableOpenMap;
34-
import org.elasticsearch.common.settings.Settings;
3532
import org.elasticsearch.index.Index;
3633
import org.elasticsearch.index.shard.ShardId;
3734
import org.elasticsearch.index.shard.ShardPath;
@@ -184,12 +181,12 @@ public void testFillDiskUsageSomeInvalidValues() {
184181
new FsInfo.Path("/most", "/dev/sdc", 300, 290, 280),
185182
};
186183
FsInfo.Path[] node2FSInfo = new FsInfo.Path[] {
187-
new FsInfo.Path("/least_most", "/dev/sda", -2, -1, -1),
184+
new FsInfo.Path("/least_most", "/dev/sda", -1, -1, -1),
188185
};
189186

190187
FsInfo.Path[] node3FSInfo = new FsInfo.Path[] {
191188
new FsInfo.Path("/most", "/dev/sda", 100, 90, 70),
192-
new FsInfo.Path("/least", "/dev/sda", 10, -8, 0),
189+
new FsInfo.Path("/least", "/dev/sda", 10, -1, 0),
193190
};
194191
List<NodeStats> nodeStats = Arrays.asList(
195192
new NodeStats(new DiscoveryNode("node_1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0,

0 commit comments

Comments
 (0)