Move Bytes setting to Java by andsel · Pull Request #18709 · elastic/logstash

andsel · 2026-02-03T15:50:45Z

Release notes

[rn:skip]

What does this PR do?

Moves the Ruby setting class Bytes setting to Java. It covers the test in unit test while keeping and adapting the original bytes_spec.rb test suite to prove it still works on Ruby code.

Why is it important/What is the impact to the user?

N/A

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files (and/or docker env variables)~~
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

[ ]

How to test this PR locally

To test locally change the default value of a setting that uses it (for example: queue.page_capacity), start Logstash and verify the change is reflected to the filesystem:

Update logstash.yml to configure a different path for PQ:

path.data: /tmp/logstash/data
queue.type: persisted
queue.page_capacity: 128mb

Run Logstas a simple pipeline:

bin/logstash -e "input{generator {message => 'Hello world'}} output{sink{}}"

Verify /tmp/logstash/data/queue/main contains PQ page files of the desired size.

Related issues

github-actions · 2026-02-03T15:50:56Z

🤖 GitHub comments

Just comment with:

run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)
/run exhaustive tests : Run the exhaustive tests Buildkite pipeline.

mergify · 2026-02-03T15:51:32Z

This pull request does not have a backport label. Could you fix it @andsel? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
If no backport is necessary, please add the backport-skip label

…nything while the Ruby implementation did as a side effect, the test was updated to retrieve the value instead of relaying on set that return a value

elasticmachine · 2026-02-04T11:36:20Z

💚 Build Succeeded

Buildkite Build
Commit: 4bacccb

History

💚 Build #4251 succeeded ed8a6f2

cc @andsel

andsel · 2026-02-04T12:19:57Z

logstash-core/spec/logstash/settings/bytes_spec.rb

    context "when given '10mb'" do
      it "returns 10485760" do
-        expect(subject.set("10mb")).to be == 10485760
+        subject.set("10mb")


Note for reviewer

in Ruby implementation the set method also return the result of the last statement, so this make it implicitly to return the set value. In Java it return void, and to avoid changing all the Java Setting hierarchy to update the set method to return a value I've updated the test code. Also I don't like to return a value in a mutator method like set, a setter is a setter and shouldn't return a value.

Copilot

Pull request overview

This pull request migrates the Ruby Setting::Bytes class to Java as BytesSetting, completing the migration started in PR #18679 which moved the ByteValue utility class. The migration maintains backward compatibility by importing the Java class into the Ruby namespace, allowing existing Ruby code to use the new Java implementation transparently.

Changes:

Implemented BytesSetting class in Java with support for string values with byte units and numeric values
Added comprehensive test coverage in BytesSettingTest.java including edge cases for BigInteger values
Enhanced ByteValue.parse() to handle non-parseable numeric strings with appropriate error handling
Updated all references from Setting::Bytes to Setting::BytesSetting in Ruby code
Adapted Ruby specs to work with the new Java implementation

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`logstash-core/src/main/java/org/logstash/settings/BytesSetting.java`	New Java implementation of the bytes setting class with coercion and validation logic
`logstash-core/src/test/java/org/logstash/settings/BytesSettingTest.java`	Comprehensive test suite covering string/numeric defaults, validation, and edge cases
`logstash-core/src/main/java/org/logstash/util/ByteValue.java`	Added error handling for non-parseable numbers with try-catch block
`logstash-core/src/test/java/org/logstash/util/ByteValueTest.java`	Added test for non-parseable number handling
`logstash-core/lib/logstash/settings.rb`	Removed Ruby Bytes class and added java_import for BytesSetting
`logstash-core/lib/logstash/environment.rb`	Updated references from Setting::Bytes to Setting::BytesSetting
`logstash-core/spec/logstash/queue_factory_spec.rb`	Updated references from Setting::Bytes to Setting::BytesSetting
`logstash-core/spec/logstash/settings/bytes_spec.rb`	Adapted test to call value() after set() to work with Java implementation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T12:20:27Z

logstash-core/src/main/java/org/logstash/settings/BytesSetting.java

+    public BytesSetting(String name, String defaultValue) {
+        super(name, coerceToNumber(defaultValue), true, BytesSetting::isValid);
+    }
+
+    public BytesSetting(String name, Number defaultValue) {
+        super(name, defaultValue, true, BytesSetting::isValid);
+    }
+
+    // Constructor for testing with strict parameter
+    public BytesSetting(String name, String defaultValue, boolean strict) {
+        super(name, coerceToNumber(defaultValue), strict, BytesSetting::isValid);


The constructors at lines 31-32 and 40-41 pre-coerce the defaultValue by calling coerceToNumber(defaultValue) before passing it to super(). However, the parent Coercible class will call coerce() again on this value when strict is true (see Coercible.java line 31). This results in unnecessary double-coercion and is inconsistent with the pattern used by other settings like IntegerSetting and NumericSetting, which pass the raw value to super() and let the parent handle coercion. Consider changing these constructors to pass the raw String or Number value directly to super(), similar to how IntegerSetting is implemented.

This is partially true because this class extends Coercible<Number> if we pass the defaultValue straight to the super constructor we need to have common root between Number and String which is Object, and it's too loose. This class can accept String and Number and coerce those.
When coerce converts the string to Number and that's passed to the super constructor, in case strict is true the second coercion is just a passthrough because the input is already a Number.

andsel · 2026-02-04T12:20:33Z

logstash-core/src/test/java/org/logstash/util/ByteValueTest.java

        assertThat(thrownException.getMessage(), containsString("Unknown bytes value '100xyz'"));
    }

+    @Test


Added this test case because wasn't covered before.

donoghuc · 2026-02-06T23:29:26Z

logstash-core/lib/logstash/settings.rb

-        when ::Numeric
-          value
-        when ::String
-          LogStash::Util::ByteValue.parse(value)


I think this was the only place where we used require "logstash/util/byte_value" in this file but it is loaded here and not anywhere else (needed in persisted_queue_config_validator.rb). Could potentially move the require there, but its probably not that big of a deal. Up to you!

andsel self-assigned this Feb 3, 2026

andsel linked an issue Feb 3, 2026 that may be closed by this pull request

Reimplement LogStash::Setting::Bytes to Java #18694

Open

andsel added 4 commits February 4, 2026 12:10

First Cursor translation

ffd8e58

[Test] Added test to verify invalid uses cases

4f45b64

Implemented validate() method and covered with tests

6aa8dad

[Test] Given the Java implementation of set() method doesn't return a…

4bacccb

…nything while the Ruby implementation did as a side effect, the test was updated to retrieve the value instead of relaying on set that return a value

andsel force-pushed the feature/bytes_setting_to_java branch from ed8a6f2 to 4bacccb Compare February 4, 2026 11:10

andsel requested a review from Copilot February 4, 2026 12:15

Copilot started reviewing on behalf of andsel February 4, 2026 12:16 View session

andsel commented Feb 4, 2026

View reviewed changes

Copilot AI reviewed Feb 4, 2026

View reviewed changes

andsel commented Feb 4, 2026

View reviewed changes

andsel marked this pull request as ready for review February 4, 2026 12:33

donoghuc requested review from donoghuc and removed request for donoghuc February 4, 2026 17:01

donoghuc approved these changes Feb 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Bytes setting to Java#18709

Move Bytes setting to Java#18709
andsel wants to merge 4 commits intoelastic:mainfrom
andsel:feature/bytes_setting_to_java

andsel commented Feb 3, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Feb 3, 2026

Uh oh!

mergify bot commented Feb 3, 2026

Uh oh!

elasticmachine commented Feb 4, 2026

Uh oh!

andsel Feb 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

andsel Feb 4, 2026

Uh oh!

andsel Feb 4, 2026

Uh oh!

donoghuc Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

andsel commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Uh oh!

github-actions bot commented Feb 3, 2026

🤖 GitHub comments

Uh oh!

mergify bot commented Feb 3, 2026

Uh oh!

elasticmachine commented Feb 4, 2026

💚 Build Succeeded

History

Uh oh!

andsel Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

andsel Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

andsel Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

donoghuc Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andsel commented Feb 3, 2026 •

edited

Loading