frankgh commented on code in PR #159: URL: https://github.com/apache/cassandra-sidecar/pull/159#discussion_r1909409247
########## server/src/test/integration/org/apache/cassandra/sidecar/routes/GossipStatusHandlerIntegrationTest.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.routes; + +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.Future; +import io.vertx.ext.web.client.predicate.ResponsePredicate; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.response.GossipStatusResponse; +import org.apache.cassandra.sidecar.testing.IntegrationTestBase; +import org.apache.cassandra.testing.CassandraIntegrationTest; +import org.apache.cassandra.testing.CassandraTestContext; + +import static io.netty.handler.codec.http.HttpResponseStatus.OK; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Test GET gossip status endpoint with C* container + */ +@ExtendWith(VertxExtension.class) +public class GossipStatusHandlerIntegrationTest extends IntegrationTestBase +{ + private static final String testRoute = "/api/v1/cassandra/gossip/status"; + + private Future<GossipStatusResponse> getGossipStatus() + { + return client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_OK) + .send() + .compose(response -> { + assertThat(response.statusCode()).isEqualTo(OK.code()); + return Future.succeededFuture(response.bodyAsJson(GossipStatusResponse.class)); + }); + } + + @CassandraIntegrationTest() + void testGossipStatus(CassandraTestContext context, VertxTestContext testContext) Review Comment: nice! ########## server-common/src/main/java/org/apache/cassandra/sidecar/common/server/StorageOperations.java: ########## @@ -102,4 +103,9 @@ default void outOfRangeDataCleanup(@NotNull String keyspace, @NotNull String tab { outOfRangeDataCleanup(keyspace, table, 1); } + + /** + * @return returns gossip status Review Comment: ```suggestion * @return the gossip status ``` ########## server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.routes; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.buffer.Buffer; +import io.vertx.core.json.JsonObject; +import io.vertx.ext.web.client.HttpResponse; +import io.vertx.ext.web.client.WebClient; +import io.vertx.ext.web.client.predicate.ResponsePredicate; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.response.GossipStatusResponse; + +import static io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR; +import static io.netty.handler.codec.http.HttpResponseStatus.OK; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link GossipInfoHandler} + */ +@ExtendWith(VertxExtension.class) +public class GossipStatusHandlerTest extends JmxCommonTest +{ + private static final String testRoute = "/api/v1/cassandra/gossip/status"; + + @Test + void testGossipRunning(VertxTestContext context) + { + when(storageOperations.gossipStatus()).thenReturn(new GossipStatusResponse("RUNNING")); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_OK) + .send(context.succeeding(response -> verifyValidResponse(context, response, "RUNNING"))); + } + + @Test + void testGossipNotRunning(VertxTestContext context) Review Comment: this is also redundant not sure what exactly we are testing here. ########## server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.routes; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.buffer.Buffer; +import io.vertx.core.json.JsonObject; +import io.vertx.ext.web.client.HttpResponse; +import io.vertx.ext.web.client.WebClient; +import io.vertx.ext.web.client.predicate.ResponsePredicate; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.response.GossipStatusResponse; + +import static io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR; +import static io.netty.handler.codec.http.HttpResponseStatus.OK; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link GossipInfoHandler} + */ +@ExtendWith(VertxExtension.class) +public class GossipStatusHandlerTest extends JmxCommonTest +{ + private static final String testRoute = "/api/v1/cassandra/gossip/status"; + + @Test + void testGossipRunning(VertxTestContext context) + { + when(storageOperations.gossipStatus()).thenReturn(new GossipStatusResponse("RUNNING")); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_OK) + .send(context.succeeding(response -> verifyValidResponse(context, response, "RUNNING"))); + } + + @Test + void testGossipNotRunning(VertxTestContext context) + { + when(storageOperations.gossipStatus()).thenReturn(new GossipStatusResponse("NOT_RUNNING")); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_OK) + .send(context.succeeding(response -> verifyValidResponse(context, response, "NOT_RUNNING"))); + } + + @Test + void testWithInstanceId(VertxTestContext context) + { + when(storageOperations.gossipStatus()).thenReturn(new GossipStatusResponse("RUNNING")); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute + "?instanceId=200") + .expect(ResponsePredicate.SC_OK) + .send(context.succeeding(response -> verifyValidResponse(context, response, "RUNNING"))); + } + + @Test + void testFailure(VertxTestContext context) + { + doThrow(new RuntimeException()).when(storageOperations).gossipStatus(); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_INTERNAL_SERVER_ERROR) + .send(context.succeeding(response -> { + assertThat(response.statusCode()).isEqualTo(INTERNAL_SERVER_ERROR.code()); + context.completeNow(); + })); + } + + private void verifyValidResponse(VertxTestContext context, HttpResponse<Buffer> response, String expectedValue) Review Comment: maybe call this `verifyAndComplete` ########## server/src/test/java/org/apache/cassandra/sidecar/routes/JmxCommonTest.java: ########## @@ -0,0 +1,123 @@ +/* + * 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.routes; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import com.google.common.collect.ImmutableList; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.google.inject.Module; +import com.google.inject.Provides; +import com.google.inject.Singleton; +import com.google.inject.util.Modules; +import io.vertx.core.Vertx; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.TestModule; +import org.apache.cassandra.sidecar.cluster.CassandraAdapterDelegate; +import org.apache.cassandra.sidecar.cluster.InstancesMetadata; +import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata; +import org.apache.cassandra.sidecar.common.server.StorageOperations; +import org.apache.cassandra.sidecar.server.MainModule; +import org.apache.cassandra.sidecar.server.Server; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Common test code to setup/teardown the server, populate test instances metadata and mocks for JMX calls + */ +class JmxCommonTest +{ + static final Logger LOGGER = LoggerFactory.getLogger(JmxCommonTest.class); + CassandraAdapterDelegate delegate = mock(CassandraAdapterDelegate.class); + StorageOperations storageOperations = mock(StorageOperations.class); + Vertx vertx; + Server server; + + @BeforeEach + void before() throws InterruptedException + { + Module testOverride = Modules.override(new TestModule()).with(new JmxTestModule()); + Injector injector = Guice.createInjector(Modules.override(new MainModule()) + .with(testOverride)); + vertx = injector.getInstance(Vertx.class); + server = injector.getInstance(Server.class); + VertxTestContext context = new VertxTestContext(); + server.start() + .onSuccess(s -> context.completeNow()) + .onFailure(context::failNow); + context.awaitCompletion(5, TimeUnit.SECONDS); + } + + @AfterEach + void after() throws InterruptedException + { + CountDownLatch closeLatch = new CountDownLatch(1); + server.close().onSuccess(res -> closeLatch.countDown()); + if (closeLatch.await(60, TimeUnit.SECONDS)) + LOGGER.info("Close event received before timeout."); + else + LOGGER.error("Close event timed out."); + } + + class JmxTestModule extends AbstractModule + { + @Provides + @Singleton + public InstancesMetadata instanceConfig() + { + int instanceId1 = 100; Review Comment: I think we should probably model this correctly if we are meaning to use this in other tests. ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.response; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Response for GET gossip status API + */ +@JsonInclude(JsonInclude.Include.NON_NULL) +public class GossipStatusResponse +{ + private final String gossipRunningStatus; + + /** + * Constructs a response with gossip status + * @param gossipRunningStatus running status of gossip + */ + public GossipStatusResponse(@JsonProperty("gossipRunningStatus") String gossipRunningStatus) + { + this.gossipRunningStatus = gossipRunningStatus; + } + + @JsonProperty("gossipRunningStatus") + public String gossipRunningStatus() Review Comment: should we add a utility method here `isRunning` that returns `"RUNNING".equals(gossipRunningStatus)`. Similar to what you had in an earlier iteration of this PR. ########## server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.routes; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.buffer.Buffer; +import io.vertx.core.json.JsonObject; +import io.vertx.ext.web.client.HttpResponse; +import io.vertx.ext.web.client.WebClient; +import io.vertx.ext.web.client.predicate.ResponsePredicate; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.response.GossipStatusResponse; + +import static io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR; +import static io.netty.handler.codec.http.HttpResponseStatus.OK; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link GossipInfoHandler} + */ +@ExtendWith(VertxExtension.class) +public class GossipStatusHandlerTest extends JmxCommonTest +{ + private static final String testRoute = "/api/v1/cassandra/gossip/status"; + + @Test + void testGossipRunning(VertxTestContext context) Review Comment: this test is redundant, not sure what we gain here. ########## server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.routes; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import io.vertx.core.buffer.Buffer; +import io.vertx.core.json.JsonObject; +import io.vertx.ext.web.client.HttpResponse; +import io.vertx.ext.web.client.WebClient; +import io.vertx.ext.web.client.predicate.ResponsePredicate; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; +import org.apache.cassandra.sidecar.common.response.GossipStatusResponse; + +import static io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR; +import static io.netty.handler.codec.http.HttpResponseStatus.OK; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link GossipInfoHandler} + */ +@ExtendWith(VertxExtension.class) +public class GossipStatusHandlerTest extends JmxCommonTest +{ + private static final String testRoute = "/api/v1/cassandra/gossip/status"; + + @Test + void testGossipRunning(VertxTestContext context) + { + when(storageOperations.gossipStatus()).thenReturn(new GossipStatusResponse("RUNNING")); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_OK) + .send(context.succeeding(response -> verifyValidResponse(context, response, "RUNNING"))); + } + + @Test + void testGossipNotRunning(VertxTestContext context) + { + when(storageOperations.gossipStatus()).thenReturn(new GossipStatusResponse("NOT_RUNNING")); + + WebClient client = WebClient.create(vertx); + client.get(server.actualPort(), "127.0.0.1", testRoute) + .expect(ResponsePredicate.SC_OK) + .send(context.succeeding(response -> verifyValidResponse(context, response, "NOT_RUNNING"))); + } + + @Test + void testWithInstanceId(VertxTestContext context) Review Comment: also, this test doesn't make a lot of sense to me. My problem with the test is that in the setup `JmxCommonTest` you share the `delegate` and `storageOperations` among 3 instances. This setup is problematic because in the real execution of sidecar each instance will have its own `delegate` and all the operations associated with a single delegate. I would drop this test, we already cover accessing different instances with the query parameter, so I would consider testing this outside of the scope of this test. ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.response; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Response for GET gossip status API + */ +@JsonInclude(JsonInclude.Include.NON_NULL) +public class GossipStatusResponse +{ + private final String gossipRunningStatus; + + /** + * Constructs a response with gossip status + * @param gossipRunningStatus running status of gossip + */ + public GossipStatusResponse(@JsonProperty("gossipRunningStatus") String gossipRunningStatus) Review Comment: I think `status` should be sufficient. We don't have a precedent for qualifying the field name like this. ########## server/src/main/java/org/apache/cassandra/sidecar/routes/GossipStatusHandler.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.routes; + +import com.google.inject.Inject; +import io.vertx.core.http.HttpServerRequest; +import io.vertx.core.net.SocketAddress; +import io.vertx.ext.web.RoutingContext; +import org.apache.cassandra.sidecar.common.server.StorageOperations; +import org.apache.cassandra.sidecar.concurrent.ExecutorPools; +import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher; + + +/** + * Handler for to retrieve gossip status Review Comment: ```suggestion * Handles gossip status retrieval ``` -- 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]
