JeetKunDoug commented on code in PR #172: URL: https://github.com/apache/cassandra-sidecar/pull/172#discussion_r1915810552
########## server-common/src/main/java/org/apache/cassandra/sidecar/common/server/utils/DurationSpec.java: ########## @@ -0,0 +1,281 @@ +/* + * 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; + +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 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 DurationSpec implements Comparable<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 DurationSpec} 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 DurationSpec(String value) throws IllegalArgumentException + { + Matcher matcher = UNITS_PATTERN.matcher(value); + + if (matcher.find()) + { + this.quantity = Long.parseLong(matcher.group(1)); + this.unit = fromSymbol(matcher.group(2)); + } + else + { + throw iae(value); + } + + validateMinUnit(value, unit, minimumUnit()); + validateQuantity(value, quantity, unit, minimumUnit(), Long.MAX_VALUE); + } + + /** + * Constructs the {@link DurationSpec} 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 DurationSpec(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(); + + /** + * @return the amount of time in the specified unit + */ + public long quantity() + { + return quantity; + } + + /** + * @return the time unit to specify the amount of duration + */ + public TimeUnit unit() + { + return unit; + } + + /** + * 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. + */ + public long to(TimeUnit targetUnit) + { + return targetUnit.convert(quantity(), unit()); + } + + /** + * @return the duration in seconds + */ + public long toSeconds() + { + return to(TimeUnit.SECONDS); + } + + /** + * {@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 + symbol(unit); + } + + /** + * {@inheritDoc} + */ + @Override + public int compareTo(@NotNull DurationSpec that) + { + if (this.unit == that.unit()) + { Review Comment: While I'm a big proponent of using curly braces all the time, given our coding style this shouldn't have curly braces. 😿 ########## server/src/main/java/org/apache/cassandra/sidecar/config/yaml/S3ClientConfigurationImpl.java: ########## @@ -67,27 +72,27 @@ public S3ClientConfigurationImpl() { this(DEFAULT_THREAD_NAME_PREFIX, DEFAULT_S3_CLIENT_CONCURRENCY, - DEFAULT_THREAD_KEEP_ALIVE_SECONDS, + DEFAULT_THREAD_KEEP_ALIVE, DEFAULT_RANGE_GET_OBJECT_BYTES_SIZE, - DEFAULT_API_CALL_TIMEOUT_MILLIS, + DEFAULT_API_CALL_TIMEOUT, new S3ProxyConfigurationImpl()); } public S3ClientConfigurationImpl(String threadNamePrefix, int concurrency, - long threadKeepAliveSeconds, + SecondBoundConfiguration threadKeepAlive, int rangeGetObjectBytesSize, - long apiCallTimeoutMillis, + MillisecondBoundConfiguration apiCallTimeout, S3ProxyConfiguration proxyConfig) { - Preconditions.checkArgument(apiCallTimeoutMillis > TimeUnit.SECONDS.toMillis(10), - "apiCallTimeout cannot be smaller than 10 seconds. Configured: " + apiCallTimeoutMillis + " ms"); + Preconditions.checkArgument(apiCallTimeout.compareTo(MINIMUM_API_CALL_TIMEOUT) > 0, + "apiCallTimeout cannot be smaller than 10 seconds. Configured: " + apiCallTimeout); Review Comment: Can we use `MINIMUM_API_CALL_TIMEOUT` in the error message here rather than the hard coded `10 seconds`? That way if one ever changes the other will also be updated. ########## server/src/main/java/org/apache/cassandra/sidecar/server/MainModule.java: ########## @@ -691,22 +698,31 @@ public ElectorateMembership electorateMembership(InstancesMetadata instancesMeta @Provides @Singleton public ClusterLeaseClaimTask clusterLeaseClaimTask(Vertx vertx, + ServiceConfiguration serviceConfiguration, ElectorateMembership electorateMembership, SidecarLeaseDatabaseAccessor accessor, - ServiceConfiguration serviceConfiguration, - PeriodicTaskExecutor periodicTaskExecutor, ClusterLease clusterLease, SidecarMetrics metrics) { - ClusterLeaseClaimTask task = new ClusterLeaseClaimTask(vertx, - serviceConfiguration, - electorateMembership, - accessor, - clusterLease, - metrics); + return new ClusterLeaseClaimTask(vertx, + serviceConfiguration, + electorateMembership, + accessor, + clusterLease, + metrics); + } + + @Provides + @Singleton + public PeriodicTaskExecutor periodicTaskExecutor(Vertx vertx, + ExecutorPools executorPools, + ClusterLease clusterLease, + ClusterLeaseClaimTask clusterLeaseClaimTask) + { + PeriodicTaskExecutor periodicTaskExecutor = new PeriodicTaskExecutor(executorPools, clusterLease); vertx.eventBus().localConsumer(ON_SIDECAR_SCHEMA_INITIALIZED.address(), - ignored -> periodicTaskExecutor.schedule(task)); - return task; + ignored -> periodicTaskExecutor.schedule(clusterLeaseClaimTask)); Review Comment: Maybe not for this PR, but I'm not really thrilled that the setup of the `clusterLeaseClaimTask` is in here... I don't think it was really any better before, but somehow starting a periodic task in a provider method like this seems inappropriate - don't really have a simple/sensical answer yet though. ########## server/src/test/java/org/apache/cassandra/sidecar/cdc/CDCLogCacheTest.java: ########## @@ -116,13 +118,18 @@ private File instance1CommitLogFile() private CdcLogCache cdcLogCache() { ExecutorPools executorPools = ExecutorPoolsHelper.createdSharedTestPool(Vertx.vertx()); - return new CdcLogCache(executorPools, instancesMetadata, 100L); + SecondBoundConfiguration mockCacheExpiryConfig = mock(SecondBoundConfiguration.class); Review Comment: Can you please document why you're using a `mock` here rather than just instantiating a DurationSpec? Took me a moment to realize you're working around the SecondBoundConfiguration's min unit, if I understand correctly. ########## server-common/src/main/java/org/apache/cassandra/sidecar/common/server/utils/MillisecondBoundConfiguration.java: ########## @@ -0,0 +1,101 @@ +/* + * 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.concurrent.TimeUnit; + +import com.fasterxml.jackson.annotation.JsonCreator; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; + +/** + * 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 class MillisecondBoundConfiguration extends DurationSpec +{ + /** + * Represents a 0-millisecond configuration + */ + public static final MillisecondBoundConfiguration ZERO = new MillisecondBoundConfiguration(0, TimeUnit.MILLISECONDS); + + /** + * Represents a 1-millisecond configuration + */ + public static final MillisecondBoundConfiguration ONE = new MillisecondBoundConfiguration(1, TimeUnit.MILLISECONDS); + + public MillisecondBoundConfiguration() + { + super(0, MILLISECONDS); + } + + @JsonCreator + public MillisecondBoundConfiguration(String value) + { + super(value); + } + + public MillisecondBoundConfiguration(long quantity, TimeUnit unit) + { + super(quantity, unit); + } + + /** + * Parses the {@code value} into a {@link MillisecondBoundConfiguration}. + * + * @param value the value to parse + * @return the parsed value into a {@link MillisecondBoundConfiguration} object + */ + public static MillisecondBoundConfiguration parse(String value) + { + return new MillisecondBoundConfiguration(value); + } + + /** + * {@inheritDoc} + */ + @Override + public TimeUnit minimumUnit() + { + return MILLISECONDS; + } + + /** + * @return the duration in milliseconds + */ + public long toMillis() + { + return to(TimeUnit.MILLISECONDS); + } Review Comment: We frequently have to convert a `DurationSpec` to milliseconds - thoughts on pulling this up to the parent class, and then replacing the `.to(TimeUnit.MILLISECONDS)` calls? -- 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]
