bbotella commented on code in PR #172:
URL: https://github.com/apache/cassandra-sidecar/pull/172#discussion_r1910495865
##########
server/src/main/java/org/apache/cassandra/sidecar/config/ServiceConfiguration.java:
##########
@@ -92,7 +92,7 @@ default List<SocketAddress> listenSocketAddresses()
/**
* @return the maximum time skew allowed between the server and the client
*/
- int allowableSkewInMinutes();
+ SecondBoundConfiguration allowableTimeSkew();
Review Comment:
Is it ok to be using `SecondBoundConfiguration` instead of
`MinuteBoundConfiguration` for this one?
##########
conf/sidecar.yaml:
##########
@@ -60,70 +60,74 @@ cassandra_instances:
sidecar:
host: 0.0.0.0
port: 9043
- request_idle_timeout_millis: 300000 # this field expects integer value
- request_timeout_millis: 300000
+ request_idle_timeout: 5m
+ request_timeout: 5m
tcp_keep_alive: false
accept_backlog: 1024
server_verticle_instances: 1
throttle:
stream_requests_per_sec: 5000
- timeout_sec: 10
+ timeout: 10s
traffic_shaping:
inbound_global_bandwidth_bps: 0 # 0 implies unthrottled, the
inbound bandwidth in bytes per second
outbound_global_bandwidth_bps: 0 # 0 implies unthrottled, the
outbound bandwidth in bytes per second
peak_outbound_global_bandwidth_bps: 419430400 # the peak outbound
bandwidth in bytes per second. The default is 400 mebibytes per second
- max_delay_to_wait_millis: 15000 # 15 seconds
- check_interval_for_stats_millis: 1000 # 1 second
+ max_delay_to_wait: 15s # 15 seconds
Review Comment:
Do we need the `15 seconds` comment anymore? Same for the rest of the
commented time units.
##########
server/src/main/java/org/apache/cassandra/sidecar/config/DurationSpec.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.config;
+
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.exceptions.ConfigurationException;
+
+import static java.util.concurrent.TimeUnit.DAYS;
+import static java.util.concurrent.TimeUnit.HOURS;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+/**
+ * Represents a positive time duration. Wrapper interface for Cassandra
Sidecar duration configuration parameters,
+ * providing to the users the opportunity to be able to provide configuration
values with a unit of their choice
Review Comment:
```suggestion
* allowing the users the opportunity to be able to provide configuration
values with a unit of their choice
```
##########
server/src/main/java/org/apache/cassandra/sidecar/config/MillisecondBoundConfiguration.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.config;
+
+import
org.apache.cassandra.sidecar.config.yaml.MillisecondBoundConfigurationImpl;
+
+/**
+ * Represents a duration used for Sidecar configuration. The bound is [0,
Long.MAX_VALUE) in milliseconds.
+ * If the user sets a different unit - we still validate that converted to
milliseconds the quantity will not exceed
+ * that upper bound.
+ */
+public interface MillisecondBoundConfiguration extends DurationSpec
+{
+ /**
+ * Represents a 0-millisecond configuration
+ */
+ MillisecondBoundConfiguration ZERO =
MillisecondBoundConfiguration.parse("0ms");
Review Comment:
Nit: wouldn't it be faster to have a constructor instead of having to parse
the string?
##########
server/src/main/java/org/apache/cassandra/sidecar/config/DurationSpec.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.config;
+
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.exceptions.ConfigurationException;
+
+import static java.util.concurrent.TimeUnit.DAYS;
+import static java.util.concurrent.TimeUnit.HOURS;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+/**
+ * Represents a positive time duration. Wrapper interface for Cassandra
Sidecar duration configuration parameters,
+ * providing to the users the opportunity to be able to provide configuration
values with a unit of their choice
+ * in {@code sidecar.yaml} as per the available options. This class mirrors
the Cassandra DurationSpec class.
+ */
+public interface DurationSpec extends Comparable<DurationSpec>
+{
+ /**
+ * @param unit the time unit
+ * @return the symbol associated with the provide time unit
+ */
+ static String symbol(TimeUnit unit)
+ {
+ switch (unit)
+ {
+ case DAYS:
+ return "d";
+ case HOURS:
+ return "h";
+ case MINUTES:
+ return "m";
+ case SECONDS:
+ return "s";
+ case MILLISECONDS:
+ return "ms";
+ }
+ throw new AssertionError();
Review Comment:
Maybe adding some text to the error?
##########
server/src/main/java/org/apache/cassandra/sidecar/config/yaml/CacheConfigurationImpl.java:
##########
@@ -41,34 +46,50 @@ public class CacheConfigurationImpl implements
CacheConfiguration
@JsonProperty("warmup_retries")
protected final int warmupRetries;
- @JsonProperty("warmup_retry_interval_millis")
- protected final long warmupRetryIntervalMillis;
+ protected MillisecondBoundConfiguration warmupRetryInterval;
public CacheConfigurationImpl()
{
- this(TimeUnit.HOURS.toMillis(1), 100, true, 5, 1000);
+ this(MillisecondBoundConfiguration.parse("1h"), 100, true, 5,
MillisecondBoundConfiguration.parse("60s"));
}
@VisibleForTesting
- public CacheConfigurationImpl(long expireAfterAccessMillis, long
maximumSize)
+ public CacheConfigurationImpl(MillisecondBoundConfiguration
expireAfterAccess, long maximumSize)
{
- this(expireAfterAccessMillis, maximumSize, true, 5, 1000);
+ this(expireAfterAccess, maximumSize, true, 5,
MillisecondBoundConfiguration.parse("60s"));
}
- public CacheConfigurationImpl(long expireAfterAccessMillis, long
maximumSize, boolean enabled, int warmupRetries, long warmupRetryIntervalMillis)
+ public CacheConfigurationImpl(MillisecondBoundConfiguration
expireAfterAccess,
+ long maximumSize,
+ boolean enabled,
+ int warmupRetries,
+ MillisecondBoundConfiguration
warmupRetryInterval)
{
- this.expireAfterAccessMillis = expireAfterAccessMillis;
+ this.expireAfterAccess = expireAfterAccess;
this.maximumSize = maximumSize;
this.enabled = enabled;
this.warmupRetries = warmupRetries;
- this.warmupRetryIntervalMillis = warmupRetryIntervalMillis;
+ this.warmupRetryInterval = warmupRetryInterval;
}
@Override
+ @JsonProperty("expire_after_access")
+ public MillisecondBoundConfiguration expireAfterAccess()
+ {
+ return expireAfterAccess;
+ }
+
+ @JsonProperty("expire_after_access")
+ public void setExpireAfterAccess(MillisecondBoundConfiguration
expireAfterAccess)
+ {
+ this.expireAfterAccess = expireAfterAccess;
+ }
+
@JsonProperty("expire_after_access_millis")
- public long expireAfterAccessMillis()
+ public void setExpireAfterAccessMillis(long expireAfterAccessMillis)
Review Comment:
Given the fact that we haven't made a release yet, wouldn't it be ok to just
remove this?
##########
server/src/main/java/org/apache/cassandra/sidecar/config/yaml/AccessControlConfigurationImpl.java:
##########
@@ -36,7 +36,8 @@ public class AccessControlConfigurationImpl implements
AccessControlConfiguratio
private static final boolean DEFAULT_ENABLED = false;
private static final List<ParameterizedClassConfiguration>
DEFAULT_AUTHENTICATORS_CONFIGURATION = Collections.emptyList();
private static final Set<String> DEFAULT_ADMIN_IDENTITIES =
Collections.emptySet();
- private static final CacheConfiguration
DEFAULT_PERMISSION_CACHE_CONFIGURATION = new
CacheConfigurationImpl(TimeUnit.HOURS.toMillis(2), 1_000);
+ private static final CacheConfiguration
DEFAULT_PERMISSION_CACHE_CONFIGURATION =
+ new CacheConfigurationImpl(MillisecondBoundConfiguration.parse("2h"),
1_000);
Review Comment:
Really nice
##########
server/src/main/java/org/apache/cassandra/sidecar/config/yaml/DurationSpecImpl.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.config.yaml;
+
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.config.DurationSpec;
+import org.apache.cassandra.sidecar.exceptions.ConfigurationException;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Represents a positive time duration. Wrapper class for Cassandra Sidecar
duration configuration parameters,
+ * providing to the users the opportunity to be able to provide configuration
values with a unit of their choice
+ * in {@code sidecar.yaml} as per the available options. This class mirrors
the Cassandra DurationSpec class.
+ */
+abstract class DurationSpecImpl implements DurationSpec
+{
+ /**
+ * The Regexp used to parse the duration provided as String.
+ */
+ private static final Pattern UNITS_PATTERN =
Pattern.compile(("^(\\d+)(d|h|s|ms|m)$"));
+
+ private final long quantity;
+ private final TimeUnit unit;
+
+ DurationSpecImpl(String value)
+ {
+ Matcher matcher = UNITS_PATTERN.matcher(value);
+
+ if (matcher.find())
+ {
+ this.quantity = Long.parseLong(matcher.group(1));
+ this.unit = DurationSpec.fromSymbol(matcher.group(2));
+ }
+ else
+ {
+ throw new ConfigurationException(String.format("Invalid duration
value %s. Only positive numbers with" +
+ "the unit of %s are
allowed", value, acceptedUnits(minimumUnit())));
+ }
+
+ validateMinUnit(unit, minimumUnit());
+ validateQuantity(value, quantity, unit, minimumUnit(), Long.MAX_VALUE);
+ }
+
+ public DurationSpecImpl(long quantity, TimeUnit unit)
+ {
+ this.quantity = quantity;
+ this.unit = unit;
+
+ validateMinUnit(unit, minimumUnit());
+ }
+
+ /**
+ * @return the minimum unit that can be represented by this type
+ */
+ public abstract TimeUnit minimumUnit();
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public long quantity()
+ {
+ return quantity;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public TimeUnit unit()
+ {
+ return unit;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String toString()
+ {
+ return "'" + quantity + "' " + unit.toString().toLowerCase();
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public int compareTo(@NotNull DurationSpec that)
+ {
+ if (this.unit == that.unit())
+ {
+ return Long.compare(this.quantity, that.quantity());
+ }
+
+ TimeUnit minUnit = this.unit.compareTo(that.unit()) < 0 ? this.unit :
that.unit();
+ return Long.compare(this.to(minUnit), that.to(minUnit));
+ }
+
+ void validateMinUnit(TimeUnit unit, TimeUnit minUnit)
+ {
+ if (unit.compareTo(minUnit) < 0)
+ throw new IllegalArgumentException(String.format("Invalid
duration: %s Accepted units:%s",
Review Comment:
```suggestion
throw new IllegalArgumentException(String.format("Invalid
duration: %s Accepted units: %s",
```
##########
server/src/main/java/org/apache/cassandra/sidecar/config/yaml/DurationSpecImpl.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.config.yaml;
+
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.config.DurationSpec;
+import org.apache.cassandra.sidecar.exceptions.ConfigurationException;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Represents a positive time duration. Wrapper class for Cassandra Sidecar
duration configuration parameters,
+ * providing to the users the opportunity to be able to provide configuration
values with a unit of their choice
+ * in {@code sidecar.yaml} as per the available options. This class mirrors
the Cassandra DurationSpec class.
+ */
+abstract class DurationSpecImpl implements DurationSpec
+{
+ /**
+ * The Regexp used to parse the duration provided as String.
+ */
+ private static final Pattern UNITS_PATTERN =
Pattern.compile(("^(\\d+)(d|h|s|ms|m)$"));
+
+ private final long quantity;
+ private final TimeUnit unit;
+
+ DurationSpecImpl(String value)
+ {
+ Matcher matcher = UNITS_PATTERN.matcher(value);
+
+ if (matcher.find())
+ {
+ this.quantity = Long.parseLong(matcher.group(1));
+ this.unit = DurationSpec.fromSymbol(matcher.group(2));
+ }
+ else
+ {
+ throw new ConfigurationException(String.format("Invalid duration
value %s. Only positive numbers with" +
+ "the unit of %s are
allowed", value, acceptedUnits(minimumUnit())));
+ }
+
+ validateMinUnit(unit, minimumUnit());
+ validateQuantity(value, quantity, unit, minimumUnit(), Long.MAX_VALUE);
+ }
+
+ public DurationSpecImpl(long quantity, TimeUnit unit)
+ {
+ this.quantity = quantity;
+ this.unit = unit;
+
+ validateMinUnit(unit, minimumUnit());
+ }
+
+ /**
+ * @return the minimum unit that can be represented by this type
+ */
+ public abstract TimeUnit minimumUnit();
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public long quantity()
+ {
+ return quantity;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public TimeUnit unit()
+ {
+ return unit;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String toString()
+ {
+ return "'" + quantity + "' " + unit.toString().toLowerCase();
Review Comment:
Why the quotes around quantity? I think it should support to be parsed back.
##########
server/src/main/java/org/apache/cassandra/sidecar/server/MainModule.java:
##########
@@ -741,7 +741,8 @@ private static InstanceMetadata buildInstanceMetadata(Vertx
vertx,
.password(cassandraInstance.jmxRolePassword())
.enableSsl(cassandraInstance.jmxSslEnabled())
.connectionMaxRetries(jmxConfiguration.maxRetries())
-
.connectionRetryDelayMillis(jmxConfiguration.retryDelayMillis())
+ // TODO: connectionRetryDelayMillis
should be a MillisecondBoundConfiguration
Review Comment:
Why postponing it?
##########
server/src/main/java/org/apache/cassandra/sidecar/config/yaml/PeriodicTaskConfigurationImpl.java:
##########
@@ -67,47 +71,74 @@ public boolean enabled()
* {@inheritDoc}
*/
@Override
- @JsonProperty("initial_delay_millis")
- public long initialDelayMillis()
+ @JsonProperty("initial_delay")
+ public MillisecondBoundConfiguration initialDelay()
{
- return initialDelayMillis;
+ if (initialDelay == null)
+ {
+ return executeInterval;
+ }
+ return initialDelay;
}
- @JsonProperty("initial_delay_millis")
- public void setInitialDelayMillis(long initialDelayMillis)
+ @JsonProperty("initial_delay")
+ public void setInitialDelay(MillisecondBoundConfiguration initialDelay)
{
- if (initialDelayMillis > 0)
+ if (initialDelay.compareTo(MillisecondBoundConfigurationImpl.ZERO) > 0)
{
- this.initialDelayMillis = initialDelayMillis;
+ this.initialDelay = initialDelay;
}
else
{
- LOGGER.warn("Invalid initialDelayMillis configuration {}, the
minimum value is 0", initialDelayMillis);
- this.initialDelayMillis = 0;
+ LOGGER.warn("Invalid initialDelay configuration {}, the minimum
value is 0", initialDelay);
+ this.initialDelay = MillisecondBoundConfigurationImpl.ZERO;
}
}
+ @JsonProperty("initial_delay_millis")
+ @Deprecated
+ public void setInitialDelayMillis(long initialDelayMillis)
+ {
+ LOGGER.warn("'initial_delay_millis' is deprecated, use 'initial_delay'
instead");
+ setInitialDelay(new
MillisecondBoundConfigurationImpl(initialDelayMillis, TimeUnit.MILLISECONDS));
+ }
+
/**
* {@inheritDoc}
*/
@Override
- @JsonProperty("execute_interval_millis")
- public long executeIntervalMillis()
+ @JsonProperty("execute_interval")
+ public MillisecondBoundConfiguration executeInterval()
{
- return executeIntervalMillis;
+ return executeInterval;
}
- @JsonProperty("execute_interval_millis")
- public void setExecuteIntervalMillis(long executeIntervalMillis)
+ @JsonProperty("execute_interval")
+ public void setExecuteInterval(MillisecondBoundConfiguration
executeInterval)
{
- if (executeIntervalMillis > 1)
+ if (executeInterval.compareTo(ONE) > 0)
{
- this.executeIntervalMillis = executeIntervalMillis;
+ this.executeInterval = executeInterval;
}
else
{
- LOGGER.warn("Invalid executeIntervalMillis configuration {}, the
minimum value is 1", executeIntervalMillis);
- this.executeIntervalMillis = 1;
+ LOGGER.warn("Invalid executeInterval configuration {}, the minimum
value is 1ms", executeInterval);
+ this.executeInterval = ONE;
}
}
+
+ /**
+ * Legacy properties {@code execute_interval_millis}, {@code
poll_freq_millis}, and {@code poll_interval_millis}
+ *
+ * @param executeIntervalMillis the interval in milliseconds
+ * @deprecated in favor of {@code execute_interval}
Review Comment:
If we finally decide to leave all the deprecated configs, I think we should
have a Jira for removing them.
##########
server/src/test/java/org/apache/cassandra/sidecar/TestModule.java:
##########
@@ -133,12 +135,15 @@ protected SidecarConfigurationImpl
abstractConfig(SslConfiguration sslConfigurat
.build();
RestoreJobConfiguration restoreJobConfiguration =
RestoreJobConfigurationImpl.builder()
-
.restoreJobTablesTtlSeconds(TimeUnit.DAYS.toSeconds(14) + 1)
+
.restoreJobTablesTtl(SecondBoundConfiguration.parse((TimeUnit.DAYS.toSeconds(14)
+ 1) + "s"))
Review Comment:
It'd be great to be able to sum DurationSpecs directly. But probably
overengineering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
