jmckenzie-dev commented on code in PR #316: URL: https://github.com/apache/cassandra-sidecar/pull/316#discussion_r2794007678
########## conf/sidecar.yaml: ########## @@ -344,6 +339,13 @@ healthcheck: initial_delay: 0ms execute_interval: 30s +# Cluster Topology Monitor settings +# Monitors Cassandra cluster topology changes and publishes events when detected +cluster_topology_monitor: + enabled: true + initial_delay: 0s + execute_interval: 1000ms # Interval between topology refresh cycles Review Comment: What led to choosing once per second as our default? Is this potentially too high or too low? Coming into this naively it's not clear to me who the consumers of this information are and whether once per second is sufficient for their needs, overkill, etc. Actionable feedback beyond my questions: adding a little more context on the comment above as to what this information is and how it's used would help provide context on why the values listed were selected. There's a lot of this in `cassandra.yaml` in the cassandra codebase if you want to see some examples. Kind of commenting on the "why" not just the "what". ########## server/src/test/java/org/apache/cassandra/sidecar/tasks/ClusterTopologyMonitorTest.java: ########## @@ -0,0 +1,495 @@ +/* + * 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.tasks; + +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.Promise; +import io.vertx.core.Vertx; +import io.vertx.junit5.Checkpoint; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.server.cluster.locator.TokenRange; +import org.apache.cassandra.sidecar.common.server.utils.MillisecondBoundConfiguration; +import org.apache.cassandra.sidecar.config.PeriodicTaskConfiguration; +import org.apache.cassandra.sidecar.config.SidecarConfiguration; +import org.apache.cassandra.sidecar.config.yaml.SidecarConfigurationImpl; +import org.apache.cassandra.sidecar.coordination.TokenRingProvider; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Unit tests for the {@link ClusterTopologyMonitor} + */ +@ExtendWith(VertxExtension.class) +class ClusterTopologyMonitorTest +{ + SidecarConfiguration mockConfiguration; + PeriodicTaskConfiguration mockClusterTopologyMonitorConfiguration; + TokenRingProvider mockTokenRingProvider; + ClusterTopologyMonitor monitor; + Vertx vertx; + + @BeforeEach + void setup(Vertx vertx) + { + this.vertx = vertx; + mockConfiguration = mock(SidecarConfiguration.class); + mockClusterTopologyMonitorConfiguration = mock(PeriodicTaskConfiguration.class); + mockTokenRingProvider = mock(TokenRingProvider.class); + + when(mockConfiguration.clusterTopologyMonitorConfiguration()).thenReturn(mockClusterTopologyMonitorConfiguration); + when(mockClusterTopologyMonitorConfiguration.initialDelay()).thenReturn(MillisecondBoundConfiguration.parse("5s")); + when(mockClusterTopologyMonitorConfiguration.executeInterval()).thenReturn(MillisecondBoundConfiguration.parse("500s")); + + monitor = new ClusterTopologyMonitor(vertx, mockTokenRingProvider, mockConfiguration); + } + + @AfterEach + void tearDown(VertxTestContext context) + { + vertx.close(context.succeedingThenComplete()); + } + + @Test + void testConfiguration() + { + assertThat(monitor.initialDelay()).isEqualTo(MillisecondBoundConfiguration.parse("5s")); + assertThat(monitor.delay()).isEqualTo(MillisecondBoundConfiguration.parse("500s")); + + when(mockClusterTopologyMonitorConfiguration.initialDelay()).thenReturn(MillisecondBoundConfiguration.parse("0s")); + when(mockClusterTopologyMonitorConfiguration.executeInterval()).thenReturn(MillisecondBoundConfiguration.parse("2000s")); + ClusterTopologyMonitor customMonitor = new ClusterTopologyMonitor(vertx, mockTokenRingProvider, mockConfiguration); + + assertThat(customMonitor.initialDelay()).isEqualTo(MillisecondBoundConfiguration.parse("0s")); + assertThat(customMonitor.delay()).isEqualTo(MillisecondBoundConfiguration.parse("2000s")); + } + + @Test + void testConfigurationFromYaml() throws IOException + { + // When running tests from the server module, working directory is the server directory + // So we need to go up one level to find conf/sidecar.yaml at project root + Path configPath = Paths.get("../conf/sidecar.yaml"); Review Comment: Thinking something along the lines of "unit test wide usable accessor to get a hold of `sidecar.yaml`". ########## server/src/main/java/org/apache/cassandra/sidecar/config/yaml/SidecarConfigurationImpl.java: ########## @@ -415,6 +429,10 @@ public static class Builder implements DataObjectBuilder<Builder, SidecarConfigu = new PeriodicTaskConfigurationImpl(true, MillisecondBoundConfiguration.ZERO, MillisecondBoundConfiguration.parse("30s")); + private PeriodicTaskConfiguration clusterTopologyMonitorConfiguration + = new PeriodicTaskConfigurationImpl(true, + MillisecondBoundConfiguration.ZERO, + MillisecondBoundConfiguration.parse("1000ms")); Review Comment: If we have raw values in `sidecar.yaml` but then have a duplicate / DRY violation here where we repeat the value, this is going to be a potential source of future bugs. Not recommending you break w/tradition here, just that maybe we consider a follow up radar to have some of these values pull from a parsed .yaml file instead of duplicating the values hard-coded like this. To be fair, as I think about this I realize we largely do the same thing in cassandra in `Config.java`. ☹️ So maybe we just keep things as they are. 🤷🏻 ########## server/src/test/java/org/apache/cassandra/sidecar/tasks/ClusterTopologyMonitorTest.java: ########## @@ -0,0 +1,495 @@ +/* + * 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.tasks; + +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.Promise; +import io.vertx.core.Vertx; +import io.vertx.junit5.Checkpoint; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.server.cluster.locator.TokenRange; +import org.apache.cassandra.sidecar.common.server.utils.MillisecondBoundConfiguration; +import org.apache.cassandra.sidecar.config.PeriodicTaskConfiguration; +import org.apache.cassandra.sidecar.config.SidecarConfiguration; +import org.apache.cassandra.sidecar.config.yaml.SidecarConfigurationImpl; +import org.apache.cassandra.sidecar.coordination.TokenRingProvider; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Unit tests for the {@link ClusterTopologyMonitor} + */ +@ExtendWith(VertxExtension.class) +class ClusterTopologyMonitorTest +{ + SidecarConfiguration mockConfiguration; + PeriodicTaskConfiguration mockClusterTopologyMonitorConfiguration; + TokenRingProvider mockTokenRingProvider; + ClusterTopologyMonitor monitor; + Vertx vertx; + + @BeforeEach + void setup(Vertx vertx) + { + this.vertx = vertx; + mockConfiguration = mock(SidecarConfiguration.class); + mockClusterTopologyMonitorConfiguration = mock(PeriodicTaskConfiguration.class); + mockTokenRingProvider = mock(TokenRingProvider.class); + + when(mockConfiguration.clusterTopologyMonitorConfiguration()).thenReturn(mockClusterTopologyMonitorConfiguration); + when(mockClusterTopologyMonitorConfiguration.initialDelay()).thenReturn(MillisecondBoundConfiguration.parse("5s")); + when(mockClusterTopologyMonitorConfiguration.executeInterval()).thenReturn(MillisecondBoundConfiguration.parse("500s")); + + monitor = new ClusterTopologyMonitor(vertx, mockTokenRingProvider, mockConfiguration); + } + + @AfterEach + void tearDown(VertxTestContext context) + { + vertx.close(context.succeedingThenComplete()); + } + + @Test + void testConfiguration() + { + assertThat(monitor.initialDelay()).isEqualTo(MillisecondBoundConfiguration.parse("5s")); + assertThat(monitor.delay()).isEqualTo(MillisecondBoundConfiguration.parse("500s")); + + when(mockClusterTopologyMonitorConfiguration.initialDelay()).thenReturn(MillisecondBoundConfiguration.parse("0s")); + when(mockClusterTopologyMonitorConfiguration.executeInterval()).thenReturn(MillisecondBoundConfiguration.parse("2000s")); + ClusterTopologyMonitor customMonitor = new ClusterTopologyMonitor(vertx, mockTokenRingProvider, mockConfiguration); + + assertThat(customMonitor.initialDelay()).isEqualTo(MillisecondBoundConfiguration.parse("0s")); + assertThat(customMonitor.delay()).isEqualTo(MillisecondBoundConfiguration.parse("2000s")); + } + + @Test + void testConfigurationFromYaml() throws IOException + { + // When running tests from the server module, working directory is the server directory + // So we need to go up one level to find conf/sidecar.yaml at project root + Path configPath = Paths.get("../conf/sidecar.yaml"); Review Comment: Will this relative pathing always hold true? Is there some kind of execution env var we could instead rely on that wouldn't be brittle? -- 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]
