bbotella commented on code in PR #172: URL: https://github.com/apache/cassandra-sidecar/pull/172#discussion_r1913344034
########## server-common/src/main/java/org/apache/cassandra/sidecar/common/server/utils/DurationSpec.java: ########## @@ -0,0 +1,132 @@ +/* + * 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.common.server.utils; + +import java.util.Arrays; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +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, + * allowing users the opportunity to configure 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 + * @throws IllegalArgumentException when the {@code unit} is unsupported + */ + 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"; + case MICROSECONDS: + return "us"; + case NANOSECONDS: + return "ns"; + } + throw new IllegalArgumentException("Unsupported unit " + unit); + } + + /** + * @param symbol the time unit symbol + * @return the time unit associated to the specified symbol + * @throws IllegalArgumentException when the {@code symbol} is unsupported + */ + static TimeUnit fromSymbol(String symbol) + { + switch (symbol.toLowerCase()) Review Comment: I think we are missing NANOSECONDS here ########## server-common/src/main/java/org/apache/cassandra/sidecar/common/server/utils/DurationSpec.java: ########## @@ -0,0 +1,132 @@ +/* + * 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.common.server.utils; + +import java.util.Arrays; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +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, + * allowing users the opportunity to configure 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 + * @throws IllegalArgumentException when the {@code unit} is unsupported + */ + 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"; + case MICROSECONDS: + return "us"; + case NANOSECONDS: + return "ns"; + } + throw new IllegalArgumentException("Unsupported unit " + unit); + } + + /** + * @param symbol the time unit symbol + * @return the time unit associated to the specified symbol + * @throws IllegalArgumentException when the {@code symbol} is unsupported + */ + static TimeUnit fromSymbol(String symbol) + { + switch (symbol.toLowerCase()) + { + case "d": + return DAYS; + case "h": + return HOURS; + case "m": + return MINUTES; + case "s": + return SECONDS; + case "ms": + return MILLISECONDS; + default: + throw new IllegalArgumentException(String.format("Unsupported time unit: %s. Supported units are: %s", + symbol, Arrays.stream(TimeUnit.values()) + .map(DurationSpec::symbol) + .collect(Collectors.joining(", ")))); + } + } + + /** + * @return the amount of time in the specified unit + */ + long quantity(); + + /** + * @return the time unit to specify the amount of duration + */ + TimeUnit unit(); + + /** + * @return the minimum unit that can be represented by this type + */ + TimeUnit minimumUnit(); + + /** + * Converts this duration spec to the {@code targetUnit}. + * Conversions from finer to coarser granularities lose precision. + * + * <p>Conversions from coarser to finer granularities with arguments + * that would numerically overflow saturate to {@link Long#MIN_VALUE} + * if negative or {@link Long#MAX_VALUE} if positive. + * + * @param targetUnit the target conversion unit + * @return the converted duration in the {@code targetUnit}, + * or {@code Long.MIN_VALUE} if conversion would negatively overflow, + * or {@code Long.MAX_VALUE} if it would positively overflow. + */ + default long to(TimeUnit targetUnit) + { + return targetUnit.convert(quantity(), unit()); + } + + /** + * @return the duration in seconds + */ + default long toSeconds() Review Comment: I understand this helper, but, isn't it a bit redundant? I think `to(TimeUnit.SECONDS)` is as readable as `toSeconds`. ########## server-common/src/main/java/org/apache/cassandra/sidecar/common/server/utils/DurationSpecImpl.java: ########## @@ -0,0 +1,197 @@ +/* + * 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.common.server.utils; + +import java.util.Arrays; +import java.util.Objects; +import java.util.concurrent.TimeUnit; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + +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, + * but it differs in that it does not support nanoseconds or microseconds. + */ +public 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; + + /** + * Constructs the {@link DurationSpecImpl} with the given {@code value}. + * + * @param value the value to parse + * @throws IllegalArgumentException when the {@code value} can't be parsed, the unit is invalid, + * or the quantity is invalid + */ + protected DurationSpecImpl(String value) throws IllegalArgumentException + { + 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 iae(value); + } + + validateMinUnit(value, unit, minimumUnit()); + validateQuantity(value, quantity, unit, minimumUnit(), Long.MAX_VALUE); + } + + /** + * Constructs the {@link DurationSpecImpl} with the given {@code quantity} and {@code unit}. + * + * @param quantity the quantity for the duration + * @param unit the unit for the duration + * @throws IllegalArgumentException when the unit is invalid or the quantity is invalid + */ + protected DurationSpecImpl(long quantity, TimeUnit unit) throws IllegalArgumentException + { + this.quantity = quantity; + this.unit = unit; + + validateMinUnit(this, unit, minimumUnit()); + validateQuantity(this, quantity, unit, minimumUnit(), Long.MAX_VALUE); + } + + /** + * @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 int hashCode() + { + // Milliseconds seems to be a reasonable tradeoff + return Objects.hash(unit.toMillis(quantity)); + } + + /** + * {@inheritDoc} + */ + @Override + public boolean equals(Object obj) + { + if (this == obj) + return true; + + if (!(obj instanceof DurationSpec)) + return false; + + DurationSpec that = (DurationSpec) obj; + if (unit == that.unit()) + return quantity == that.quantity(); + + // Due to overflows we can only guarantee that the 2 durations are equal if we get the same results + // doing the conversion in both directions. + return unit.convert(that.quantity(), that.unit()) == quantity + && that.unit().convert(quantity, unit) == that.quantity(); + } + + /** + * {@inheritDoc} + */ + @Override + public String toString() + { + return quantity + DurationSpec.symbol(unit); + } + + /** + * {@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(Object value, TimeUnit unit, TimeUnit minUnit) + { + if (unit.compareTo(minUnit) < 0) + throw iae(value); + } + + void validateQuantity(Object value, long quantity, TimeUnit sourceUnit, TimeUnit minUnit, long max) Review Comment: Do we need `max` as a parameter? I think it is always `Long.MAX_VALUE` right? ########## server/src/main/java/org/apache/cassandra/sidecar/config/yaml/CacheConfigurationImpl.java: ########## @@ -41,34 +46,57 @@ 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")); Review Comment: Is the `1000ms -> 60s` change intended? ########## server/src/main/java/org/apache/cassandra/sidecar/config/yaml/CacheConfigurationImpl.java: ########## @@ -41,34 +46,57 @@ 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")); Review Comment: Is the `1000ms -> 60s` change intended? -- 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]
