Skip to content

Commit 7c7eff9

Browse files
andselandsel
authored
Moves TimeValue setting to java (#18760)
Moves the Ruby setting class TimeValue setting to Java. It covers the test in unit test while keeping and adapting the original time_value_spec.rb test suite to prove it still works on Ruby code.
1 parent 7517296 commit 7c7eff9

File tree

9 files changed

+168
-36
lines changed

9 files changed

+168
-36
lines changed

‎logstash-core/lib/logstash/environment.rb‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def self.as_java_range(r)
4545
Setting::NullableStringSetting.new("config.string", nil, false),
4646
Setting::BooleanSetting.new("config.test_and_exit", false),
4747
Setting::BooleanSetting.new("config.reload.automatic", false),
48-
Setting::TimeValue.new("config.reload.interval", "3s"), # in seconds
48+
Setting::TimeValueSetting.new("config.reload.interval", "3s"), # in seconds
4949
Setting::BooleanSetting.new("config.support_escapes", false),
5050
Setting::StringSetting.new("config.field_reference.escape_style", "none", true, %w(none percent ampersand)),
5151
Setting::BooleanSetting.new("metric.collect", true),
@@ -103,10 +103,10 @@ def self.as_java_range(r)
103103
Setting::NumericSetting.new("dead_letter_queue.flush_interval", 5000),
104104
Setting::StringSetting.new("dead_letter_queue.storage_policy", "drop_newer", true, ["drop_newer", "drop_older"]),
105105
Setting::NullableStringSetting.new("dead_letter_queue.retain.age"), # example 5d
106-
Setting::TimeValue.new("slowlog.threshold.warn", "-1"),
107-
Setting::TimeValue.new("slowlog.threshold.info", "-1"),
108-
Setting::TimeValue.new("slowlog.threshold.debug", "-1"),
109-
Setting::TimeValue.new("slowlog.threshold.trace", "-1"),
106+
Setting::TimeValueSetting.new("slowlog.threshold.warn", "-1"),
107+
Setting::TimeValueSetting.new("slowlog.threshold.info", "-1"),
108+
Setting::TimeValueSetting.new("slowlog.threshold.debug", "-1"),
109+
Setting::TimeValueSetting.new("slowlog.threshold.trace", "-1"),
110110
Setting::StringSetting.new("keystore.classname", "org.logstash.secret.store.backend.JavaKeyStore"),
111111
Setting::StringSetting.new("keystore.file", ::File.join(::File.join(LogStash::Environment::LOGSTASH_HOME, "config"), "logstash.keystore"), false), # will be populated on
112112
Setting::NullableStringSetting.new("monitoring.cluster_uuid"),

‎logstash-core/lib/logstash/settings.rb‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -456,25 +456,7 @@ def validate(value)
456456

457457
java_import org.logstash.settings.BytesSetting
458458

459-
class TimeValue < Coercible
460-
include LogStash::Util::Loggable
461-
462-
def initialize(name, default, strict = true, &validator_proc)
463-
super(name, Util::TimeValue, default, strict, &validator_proc)
464-
end
465-
466-
def coerce(value)
467-
if value.is_a?(::Integer)
468-
deprecation_logger.deprecated("Integer value for `#{name}` does not have a time unit and will be interpreted in nanoseconds. " +
469-
"Time units will be required in a future release of Logstash. " +
470-
"Acceptable unit suffixes are: `d`, `h`, `m`, `s`, `ms`, `micros`, and `nanos`.")
471-
472-
return Util::TimeValue.new(value, :nanosecond)
473-
end
474-
475-
Util::TimeValue.from_value(value)
476-
end
477-
end
459+
java_import org.logstash.settings.TimeValueSetting
478460

479461
class ArrayCoercible < Coercible
480462
def initialize(name, klass, default, strict = true, &validator_proc)

‎logstash-core/spec/logstash/settings/time_value_spec.rb‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
require "spec_helper"
1919
require "logstash/settings"
2020

21-
describe LogStash::Setting::TimeValue do
21+
describe LogStash::Setting::TimeValueSetting do
2222
subject { described_class.new("option", "-1") }
2323
describe "#set" do
2424
it "should coerce the default correctly" do
@@ -40,16 +40,10 @@
4040
end
4141

4242
context "when a value is given as a nanosecond" do
43-
let(:deprecation_logger_stub) { double("DeprecationLogger").as_null_object }
44-
before(:each) do
45-
allow(subject).to receive(:deprecation_logger).and_return(deprecation_logger_stub)
46-
end
4743
it "should set the value" do
4844
subject.set(5)
4945
expect(subject.value).to eq(LogStash::Util::TimeValue.new(5, :nanosecond))
5046
expect(subject.value.to_nanos).to eq(5)
51-
52-
expect(deprecation_logger_stub).to have_received(:deprecated).with(/units will be required/).once
5347
end
5448
end
5549
end

‎logstash-core/src/main/java/org/logstash/settings/TimeValueSetting.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/*
2+
* Licensed to Elasticsearch B.V. under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch B.V. licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.logstash.settings;
20+
21+
import co.elastic.logstash.api.DeprecationLogger;
22+
import org.apache.logging.log4j.LogManager;
23+
import org.apache.logging.log4j.Logger;
24+
import org.logstash.RubyUtil;
25+
import org.logstash.log.DefaultDeprecationLogger;
26+
import org.logstash.util.TimeValue;
27+
28+
import java.math.BigInteger;
29+
30+
/**
31+
* A setting that represents a time value with units (e.g., "18m", "5s", "100ms").
32+
* Accepts strings with time units or integer values (interpreted as nanoseconds with deprecation warning).
33+
*/
34+
public class TimeValueSetting extends Coercible<TimeValue> {
35+
36+
private static final Logger LOGGER = LogManager.getLogger();
37+
private static final DeprecationLogger DEPRECATION_LOGGER = new DefaultDeprecationLogger(LOGGER);
38+
39+
public TimeValueSetting(String name, String defaultValue) {
40+
super(name, coerceStatic(name, defaultValue), true, noValidator());
41+
}
42+
43+
@Override
44+
public TimeValue coerce(Object value) {
45+
return coerceStatic(getName(), value);
46+
}
47+
48+
private static TimeValue coerceStatic(String name, Object value) {
49+
if (value instanceof Integer || value instanceof Long || value instanceof BigInteger) {
50+
DEPRECATION_LOGGER.deprecated(
51+
"Integer value for `" + name + "` does not have a time unit and will be interpreted in nanoseconds. " +
52+
"Time units will be required in a future release of Logstash. " +
53+
"Acceptable unit suffixes are: `d`, `h`, `m`, `s`, `ms`, `micros`, and `nanos`.");
54+
if (((Number) value).longValue() > Integer.MAX_VALUE) {
55+
throw RubyUtil.RUBY.newArgumentError(
56+
"Numeric value for `" + name + "` exceeds the maximum int (" + Integer.MAX_VALUE +
57+
") supported value for nanoseconds.");
58+
}
59+
return new TimeValue(((Number) value).intValue(), "nanosecond");
60+
} else if (value instanceof Number) {
61+
throw RubyUtil.RUBY.newArgumentError(
62+
"Non-integer numeric value for `" + name + "` is not supported without a time unit. " +
63+
"Please specify a time unit suffix such as `d`, `h`, `m`, `s`, `ms`, `micros`, or `nanos`.");
64+
}
65+
return TimeValue.fromValue(value);
66+
}
67+
}

‎logstash-core/src/test/java/org/logstash/settings/TimeValueSettingTest.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/*
2+
* Licensed to Elasticsearch B.V. under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch B.V. licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.logstash.settings;
20+
21+
import org.apache.logging.log4j.core.test.appender.ListAppender;
22+
import org.apache.logging.log4j.core.test.junit.LoggerContextRule;
23+
import org.jruby.exceptions.ArgumentError;
24+
import org.junit.Before;
25+
import org.junit.ClassRule;
26+
import org.junit.Test;
27+
28+
import static org.assertj.core.api.Assertions.assertThat;
29+
import static org.junit.Assert.assertEquals;
30+
import static org.junit.Assert.assertThrows;
31+
import static org.junit.Assert.assertTrue;
32+
33+
public class TimeValueSettingTest {
34+
35+
private static final String CONFIG = "log4j2-test1.xml";
36+
37+
@ClassRule
38+
public static LoggerContextRule CTX = new LoggerContextRule(CONFIG);
39+
40+
private ListAppender appender;
41+
42+
@Before
43+
public void setUp() {
44+
appender = CTX.getListAppender("EventLogger").clear();
45+
}
46+
47+
@Test
48+
public void testDefaultValueCoercesCorrectly() {
49+
TimeValueSetting setting = new TimeValueSetting("option", "-1");
50+
assertEquals(-1L, setting.value().toNanos());
51+
}
52+
53+
@Test
54+
public void testInvalidStringValueThrowsArgumentError() {
55+
TimeValueSetting setting = new TimeValueSetting("option", "-1");
56+
assertThrows(org.jruby.exceptions.ArgumentError.class, () -> setting.set("invalid"));
57+
}
58+
59+
@Test
60+
public void testInvalidFloatValueThrowsArgumentError() {
61+
TimeValueSetting setting = new TimeValueSetting("option", "-1");
62+
assertThrows(org.jruby.exceptions.ArgumentError.class, () -> setting.set(5.5));
63+
}
64+
65+
@Test
66+
public void testSetWithTimeValueString() {
67+
TimeValueSetting setting = new TimeValueSetting("option", "-1");
68+
setting.set("18m");
69+
assertEquals(18L * 60 * 1_000_000_000L, setting.value().toNanos());
70+
}
71+
72+
@Test
73+
public void testSetWithIntegerValueAsNanosecondsLogsADeprecationMessage() {
74+
TimeValueSetting setting = new TimeValueSetting("option", "-1");
75+
setting.set(5);
76+
assertEquals(5L, setting.value().toNanos());
77+
78+
boolean printStalling = appender.getMessages().stream().anyMatch((msg) -> msg.contains("Time units will be required in a future release of Logstash."));
79+
assertTrue(printStalling);
80+
}
81+
82+
@Test
83+
public void givenNumberExceedingMaxInt_whenSet_thenThrowsArgumentError() {
84+
TimeValueSetting setting = new TimeValueSetting("option", "-1");
85+
long valueExceedingMaxInt = (long) Integer.MAX_VALUE + 1;
86+
ArgumentError ex = assertThrows(ArgumentError.class, () -> setting.set(valueExceedingMaxInt));
87+
assertThat(ex.getMessage()).contains("exceeds the maximum int");
88+
}
89+
}

‎x-pack/lib/config_management/extension.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ def additionals_settings(settings)
2222
logger.trace("Registering additionals settings")
2323

2424
settings.register(LogStash::Setting::BooleanSetting.new("xpack.management.enabled", false))
25-
settings.register(LogStash::Setting::TimeValue.new("xpack.management.logstash.poll_interval", "5s"))
25+
settings.register(LogStash::Setting::TimeValueSetting.new("xpack.management.logstash.poll_interval", "5s"))
2626
settings.register(LogStash::Setting::ArrayCoercible.new("xpack.management.pipeline.id", String, ["main"]))
2727
settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.username", "logstash_system"))
2828
settings.register(LogStash::Setting::NullableStringSetting.new("xpack.management.elasticsearch.password"))

‎x-pack/lib/geoip_database_management/extension.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ def additionals_settings(settings)
1313
logger.trace("Registering additional geoip settings")
1414
settings.register(LogStash::Setting::StringSetting.new("xpack.geoip.downloader.endpoint", "https://geoip.elastic.co/v1/database")
1515
.with_deprecated_alias("xpack.geoip.download.endpoint"))
16-
settings.register(LogStash::Setting::TimeValue.new("xpack.geoip.downloader.poll.interval", "24h"))
16+
settings.register(LogStash::Setting::TimeValueSetting.new("xpack.geoip.downloader.poll.interval", "24h"))
1717
settings.register(LogStash::Setting::BooleanSetting.new("xpack.geoip.downloader.enabled", true))
1818
rescue => e
1919
logger.error("Cannot register new settings", :message => e.message, :backtrace => e.backtrace)

‎x-pack/lib/monitoring/monitoring.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -276,8 +276,8 @@ def register_monitoring_settings(settings, prefix = "")
276276
end
277277
settings.register(LogStash::Setting::BooleanSetting.new("#{prefix}monitoring.enabled", false))
278278
settings.register(LogStash::Setting::ArrayCoercible.new("#{prefix}monitoring.elasticsearch.hosts", String, ["http://localhost:9200"]))
279-
settings.register(LogStash::Setting::TimeValue.new("#{prefix}monitoring.collection.interval", "10s"))
280-
settings.register(LogStash::Setting::TimeValue.new("#{prefix}monitoring.collection.timeout_interval", "10m"))
279+
settings.register(LogStash::Setting::TimeValueSetting.new("#{prefix}monitoring.collection.interval", "10s"))
280+
settings.register(LogStash::Setting::TimeValueSetting.new("#{prefix}monitoring.collection.timeout_interval", "10m"))
281281
settings.register(LogStash::Setting::NullableStringSetting.new("#{prefix}monitoring.elasticsearch.username", "logstash_system"))
282282
settings.register(LogStash::Setting::NullableStringSetting.new("#{prefix}monitoring.elasticsearch.password"))
283283
settings.register(LogStash::Setting::NullableStringSetting.new("#{prefix}monitoring.elasticsearch.proxy"))

‎x-pack/spec/config_management/extension_spec.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
describe "#additionals_settings" do
3030
define_settings(
3131
"xpack.management.enabled" => [LogStash::Setting::BooleanSetting, false],
32-
"xpack.management.logstash.poll_interval" => [LogStash::Setting::TimeValue, LogStash::Util::TimeValue.from_value("5s")],
32+
"xpack.management.logstash.poll_interval" => [LogStash::Setting::TimeValueSetting, LogStash::Util::TimeValue.from_value("5s")],
3333
"xpack.management.pipeline.id" => [LogStash::Setting::ArrayCoercible, ["main"]],
3434
"xpack.management.elasticsearch.hosts" => [LogStash::Setting::ArrayCoercible, ["https://localhost:9200"]],
3535
"xpack.management.elasticsearch.username" => [LogStash::Setting::StringSetting, "logstash_system"],

0 commit comments

Comments
 (0)