Fixes ByteSizeValue to serialise correctly #27702

colings86 · 2017-12-07T14:00:39Z

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

colings86 · 2017-12-08T12:20:17Z

jenkins please retest this

jasontedor

I left a few comments.

jasontedor · 2017-12-11T16:32:45Z

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

+        if (out.getVersion().before(Version.V_6_2_0)) {
+            out.writeVLong(getBytes());
+        } else {
+            out.writeZLong(getBytes());


I think we can do better here? Write out the bytes and write out the unit and read them on de-serialization to preserve the unit?

Agreed. see 74273ea

jasontedor · 2017-12-11T16:33:14Z

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

+        if (size <= 0) {
+            return String.valueOf(size);
+        }
+        return getBytes() + ByteSizeUnit.BYTES.getSuffix();


Why not size and unit?

We can do this but to maintain backward compatibility I think we need to keep the parse method converting the unit to bytes for 6.x and can only make it keep the unit in a followup PR for only the master branch. It also means that we can't compare the string representations in BizeSizeValueTests#testParse[1] since the parsed version will always be in bytes but the original will be in any unit. I think this is ok because we chack the original and the parsed version are equal but I wanted to point it out. Does this sound ok to you? (see 74273ea)

[1] https://github.com/elastic/elasticsearch/pull/27702/files#diff-6be961ed1a7543ed4fff562bef6b5680R240

Actually I think I have got a bwc to change the parsing to preserve the unit as well. To keep it bwc I've made it revert to converting the unit to bytes if the input value is fractional. see f00c3bf

colings86 · 2017-12-12T13:49:13Z

@jasontedor I pushed a couple of commits to address your comments. Could you take another look?

colings86 · 2017-12-12T15:16:33Z

jenkins retest this

jasontedor

LGTM.

jpountz

I left some questions but the change looks good.

jpountz · 2017-12-13T08:40:39Z

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

@@ -105,26 +134,33 @@ public double getPbFrac() {
        return ((double) getBytes()) / ByteSizeUnit.C5;
    }

+    public String getStringRep() {


Please add javadocs to explain how this differs from toString()

Agreed, see ad16a5c

jpountz · 2017-12-13T08:45:10Z

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

+        if (size > Long.MAX_VALUE / unit.toBytes(1)) {
+            throw new IllegalArgumentException(
+                    "Values greater than " + Long.MAX_VALUE + " bytes are not supported: " + size + unit.getSuffix());
+        }


should it also fail if size is < -1 or if size == -1 and unit != BYTES?

Agreed, see ad16a5c

colings86 · 2017-12-13T10:11:55Z

@jpountz @jasontedor thanks for the reviews. I've addressed the last review comments and will merge this when I get a green build

colings86 · 2017-12-13T11:54:40Z

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

        };

        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),


@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?

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.

jasontedor

LGTM.

colings86 · 2017-12-14T08:40:00Z

jenkins test this please

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

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.

This will be changed to 6.2 when the fix has been backported

* 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

This reverts commit c355677.

* 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

colings86 · 2017-12-14T15:45:22Z

Backport to 6.x is done but leaving the label as a reminder to change the bwc version on master to 6.2.0 once the builds have caught up with the backport

cbuescher · 2017-12-15T07:58:31Z

@colings86 I went ahead and pushed 54b1fed to correct the bwc version on master because I think mixed cluster tests were failing, hope this was okay. Can you check briefly.

colings86 · 2017-12-15T08:43:01Z

@cbuescher thanks for doing that.

We deprecated fractional byte size values in 6.2 with PR #27702 However, we didn't add a related item to the 6.2 deprecation docs. This adds the missing item to the 7.15 deprecation docs.

…77917) We deprecated fractional byte size values in 6.2 with PR #27702 However, we didn't add a related item to the 6.2 deprecation docs. This adds the missing item to the 7.15 deprecation docs.

colings86 added :Core/Infra/Core Core issues without another label >bug v6.2.0 v7.0.0 labels Dec 7, 2017

colings86 self-assigned this Dec 7, 2017

colings86 requested review from jpountz and jasontedor December 7, 2017 14:00

jasontedor requested changes Dec 11, 2017

View reviewed changes

jasontedor approved these changes Dec 13, 2017

View reviewed changes

jpountz approved these changes Dec 13, 2017

View reviewed changes

colings86 commented Dec 13, 2017

View reviewed changes

jasontedor approved these changes Dec 13, 2017

View reviewed changes

colings86 added 8 commits December 14, 2017 08:42

Fix bytes settings default value to not use fractional values

9d23322

Fixes test

960cfa1

Addresses review comments

f8792f6

Modifies parsing to preserve unit

677c869

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

b6501da

Fixes tests

f49815f

Temporarily changes version check to 7.0.0

6753cd3

This will be changed to 6.2 when the fix has been backported

colings86 merged commit 579d1fe into elastic:master Dec 14, 2017

colings86 added the backport pending label Dec 14, 2017

colings86 added a commit that referenced this pull request Dec 14, 2017

Revert "Fixes ByteSizeValue to serialise correctly (#27702)"

fb47e81

This reverts commit c355677.

colings86 removed the backport pending label Dec 15, 2017

colings86 deleted the bugfix/bytesizevalue branch December 21, 2017 12:00

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

jasontedor mentioned this pull request Mar 21, 2020

Remove support for fractional byte size values #53927

Closed

pgomulka mentioned this pull request Feb 4, 2021

Test failures around deprecating negative ByteSizeValue #68447

Closed

jrodewig mentioned this pull request Sep 15, 2021

[DOCS] Add deprecation docs for fractional byte size values #77795

Merged

Fixes ByteSizeValue to serialise correctly #27702

Fixes ByteSizeValue to serialise correctly #27702

Uh oh!

Conversation

colings86 commented Dec 7, 2017

Uh oh!

colings86 commented Dec 8, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Dec 12, 2017

Uh oh!

colings86 commented Dec 12, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

colings86 commented Dec 14, 2017

Uh oh!

colings86 commented Dec 14, 2017

Uh oh!

cbuescher commented Dec 15, 2017

Uh oh!

colings86 commented Dec 15, 2017

Uh oh!

Uh oh!

colings86 Dec 12, 2017 •

edited

Loading

colings86 Dec 12, 2017 •

edited

Loading

colings86 Dec 12, 2017 •

edited

Loading