jyothsnakonisa commented on code in PR #272: URL: https://github.com/apache/cassandra-sidecar/pull/272#discussion_r2624587881
########## adapters/adapters-base/src/test/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperationsTest.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.adapters.base; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.apache.cassandra.sidecar.adapters.base.jmx.CompactionManagerJmxOperations; +import org.apache.cassandra.sidecar.common.server.JmxClient; + +import static org.apache.cassandra.sidecar.adapters.base.jmx.CompactionManagerJmxOperations.COMPACTION_MANAGER_OBJ_NAME; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link CassandraCompactionManagerOperations} class + */ +class CassandraCompactionManagerOperationsTest +{ + private CassandraCompactionManagerOperations compactionManagerOperations; + private JmxClient mockJmxClient; + private CompactionManagerJmxOperations mockJmxOperations; + + @BeforeEach + void setUp() + { + mockJmxClient = mock(JmxClient.class); + mockJmxOperations = mock(CompactionManagerJmxOperations.class); + compactionManagerOperations = new CassandraCompactionManagerOperations(mockJmxClient); + + // Setup JMX proxy mock + when(mockJmxClient.proxy(CompactionManagerJmxOperations.class, COMPACTION_MANAGER_OBJ_NAME)) + .thenReturn(mockJmxOperations); + } + + @Test + void testStopCompactionByIdOnly() + { + // Test stopCompactionById called when providing compactionId + String compactionId = "abc-123"; + compactionManagerOperations.stopCompaction(compactionId); + + verify(mockJmxOperations, times(1)).stopCompactionById(compactionId); + verify(mockJmxOperations, times(0)).stopCompaction(org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void testStopCompactionByTypeOnly() + { + // Test stopCompaction called when no compactionId provided + String compactionType = "COMPACTION"; + compactionManagerOperations.stopCompaction(compactionType); + + verify(mockJmxOperations, times(1)).stopCompaction(compactionType); + verify(mockJmxOperations, times(0)).stopCompactionById(org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void testStopCompactionBothProvided() + { + // Test compactionId takes precedence when both compactionId and compactionType provided + String compactionId = "xyz-456"; + String compactionType = "VALIDATION"; + compactionManagerOperations.stopCompaction(compactionType); + + verify(mockJmxOperations, times(1)).stopCompactionById(compactionId); + verify(mockJmxOperations, times(0)).stopCompaction(org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void testStopCompactionByIdWithWhitespace() + { + // Test trim does not result in empty string + String compactionId = " abc-123 "; + compactionManagerOperations.stopCompaction(compactionId); + + verify(mockJmxOperations, times(1)).stopCompactionById(compactionId); + verify(mockJmxOperations, times(0)).stopCompaction(org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void testStopCompactionAllSupportedTypes() + { + // Test no failures upon any supported type being provided as param + String[] supportedTypes = { + "COMPACTION", "VALIDATION", "KEY_CACHE_SAVE", "ROW_CACHE_SAVE", + "COUNTER_CACHE_SAVE", "CLEANUP", "SCRUB", "UPGRADE_SSTABLES", + "INDEX_BUILD", "TOMBSTONE_COMPACTION", "UNKNOWN", "ANTICOMPACTION", + "VERIFY", "VIEW_BUILD", "INDEX_SUMMARY", "RELOCATE", + "GARBAGE_COLLECT", "WRITE" + }; + + for (String type : supportedTypes) + { + compactionManagerOperations.stopCompaction(type); + verify(mockJmxOperations, times(1)).stopCompaction(type); + } + } + + @Test + void testStopCompactionJmxProxyCalledOnce() + { + // Test JMX proxy obtained exactly once per call + compactionManagerOperations.stopCompactionById("test-id"); + + verify(mockJmxClient, times(1)) + .proxy(CompactionManagerJmxOperations.class, COMPACTION_MANAGER_OBJ_NAME); + } +} Review Comment: Please add a new line in the end of the file ########## server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java: ########## @@ -0,0 +1,168 @@ +/* + * 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.handlers; + +import java.util.Collections; +import java.util.Set; + +import com.google.inject.Inject; +import io.netty.handler.codec.http.HttpResponseStatus; +import io.vertx.core.http.HttpServerRequest; +import io.vertx.core.json.DecodeException; +import io.vertx.core.json.Json; +import io.vertx.core.net.SocketAddress; +import io.vertx.ext.auth.authorization.Authorization; +import io.vertx.ext.web.RoutingContext; +import org.apache.cassandra.sidecar.acl.authorization.BasicPermissions; +import org.apache.cassandra.sidecar.common.data.CompactionStopStatus; +import org.apache.cassandra.sidecar.common.data.CompactionType; +import org.apache.cassandra.sidecar.common.request.data.CompactionStopRequestPayload; +import org.apache.cassandra.sidecar.common.response.CompactionStopResponse; +import org.apache.cassandra.sidecar.common.server.CompactionManagerOperations; +import org.apache.cassandra.sidecar.concurrent.ExecutorPools; +import org.apache.cassandra.sidecar.utils.CassandraInputValidator; +import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher; +import org.jetbrains.annotations.NotNull; + +import static org.apache.cassandra.sidecar.utils.HttpExceptions.wrapHttpException; + +/** + * Handler for stopping compaction operations via the Cassandra Compaction Manager. + * + * <p>Handles {@code POST /api/v1/cassandra/operations/compaction/stop} requests to stop + * compaction operations. Expects a JSON payload with compaction_type and/or compaction_id: + * <pre> + * { "compaction_type": "COMPACTION", "compaction_id": "abc-123" } + * </pre> + */ +public class CompactionStopHandler extends AbstractHandler<CompactionStopRequestPayload> implements AccessProtected +{ + /** + * Constructs a handler with the provided {@code metadataFetcher} + * + * @param metadataFetcher the metadata fetcher + * @param executorPools executor pools for blocking executions + * @param validator validator for Cassandra-specific input + */ + @Inject + protected CompactionStopHandler(final InstanceMetadataFetcher metadataFetcher, + final ExecutorPools executorPools, + final CassandraInputValidator validator) { + super(metadataFetcher, executorPools, validator); + } + + @Override + public Set<Authorization> requiredAuthorizations() { + return Collections.singleton(BasicPermissions.MODIFY_COMPACTION.toAuthorization()); + } + + /** + * {@inheritDoc} + */ + @Override + public void handleInternal(RoutingContext context, + HttpServerRequest httpRequest, + @NotNull String host, + SocketAddress remoteAddress, + CompactionStopRequestPayload request) { + CompactionManagerOperations compactionManagerOps = metadataFetcher.delegate(host).compactionManagerOperations(); + + executorPools.service() + .executeBlocking(() -> stopCompaction(compactionManagerOps, request)) + .onSuccess(context::json) + .onFailure(cause -> processFailure(cause, context, host, remoteAddress, request)); + } + + /** + * Stops the compaction based on the request parameters + * + * @param operations the compaction manager operations + * @param request the request payload containing compaction_type and/or compaction_id + * @return CompactionStopResponse with the operation result + */ + private CompactionStopResponse stopCompaction(CompactionManagerOperations operations, + CompactionStopRequestPayload request) + { + CompactionType compactionType = request.compactionType(); + String compactionId = request.compactionId(); Review Comment: Do you want to trim the compactionID? I see that you are doing it in the other parts of the code ########## client-common/src/test/java/org/apache/cassandra/sidecar/common/request/data/CompactionStopRequestPayloadTest.java: ########## @@ -0,0 +1,179 @@ +/* + * 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.request.data; + +import org.junit.jupiter.api.Test; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.cassandra.sidecar.common.data.CompactionType; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for {@link CompactionStopRequestPayload} serialization and deserialization + */ +class CompactionStopRequestPayloadTest +{ + private static final ObjectMapper MAPPER = new ObjectMapper() + .setSerializationInclusion(JsonInclude.Include.NON_NULL); + + @Test + void testSerDeserWithBothFields() throws JsonProcessingException + { + CompactionStopRequestPayload payload = new CompactionStopRequestPayload(CompactionType.COMPACTION, "abc-123"); + String json = MAPPER.writeValueAsString(payload); + assertThat(json).isEqualTo("{\"compaction_type\":\"COMPACTION\",\"compaction_id\":\"abc-123\"}"); + + CompactionStopRequestPayload deser = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(deser.compactionType()).isEqualTo(payload.compactionType()); + assertThat(deser.compactionId()).isEqualTo(payload.compactionId()); + } + + @Test + void testSerDeserWithTypeOnly() throws JsonProcessingException + { + CompactionStopRequestPayload payload = new CompactionStopRequestPayload(CompactionType.VALIDATION, null); + String json = MAPPER.writeValueAsString(payload); + assertThat(json).isEqualTo("{\"compaction_type\":\"VALIDATION\"}"); + + CompactionStopRequestPayload deser = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(deser.compactionType()).isEqualTo(CompactionType.VALIDATION); + assertThat(deser.compactionId()).isNull(); + } + + @Test + void testSerDeserWithIdOnly() throws JsonProcessingException + { + CompactionStopRequestPayload payload = new CompactionStopRequestPayload(null, "xyz-456"); + String json = MAPPER.writeValueAsString(payload); + assertThat(json).isEqualTo("{\"compaction_id\":\"xyz-456\"}"); + + CompactionStopRequestPayload deser = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(deser.compactionType()).isNull(); + assertThat(deser.compactionId()).isEqualTo("xyz-456"); + } + + @Test + void testSerDeserWithBothNull() throws JsonProcessingException + { + CompactionStopRequestPayload payload = new CompactionStopRequestPayload(null, null); + String json = MAPPER.writeValueAsString(payload); + assertThat(json).isEqualTo("{}"); + + CompactionStopRequestPayload deser = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(deser.compactionType()).isNull(); + assertThat(deser.compactionId()).isNull(); + } + + @Test + void testDeserFromJsonWithBothFields() throws JsonProcessingException + { + String json = "{\"compaction_type\":\"CLEANUP\",\"compaction_id\":\"test-123\"}"; + CompactionStopRequestPayload payload = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(payload.compactionType()).isEqualTo(CompactionType.CLEANUP); + assertThat(payload.compactionId()).isEqualTo("test-123"); + } + + @Test + void testDeserFromJsonWithTypeOnly() throws JsonProcessingException + { + String json = "{\"compaction_type\":\"SCRUB\"}"; + CompactionStopRequestPayload payload = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(payload.compactionType()).isEqualTo(CompactionType.SCRUB); + assertThat(payload.compactionId()).isNull(); + } + + @Test + void testDeserFromJsonWithIdOnly() throws JsonProcessingException + { + String json = "{\"compaction_id\":\"unique-compaction-id\"}"; + CompactionStopRequestPayload payload = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(payload.compactionType()).isNull(); + assertThat(payload.compactionId()).isEqualTo("unique-compaction-id"); + } + + @Test + void testDeserFromEmptyJson() throws JsonProcessingException + { + String json = "{}"; + CompactionStopRequestPayload payload = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(payload.compactionType()).isNull(); + assertThat(payload.compactionId()).isNull(); + } + + @Test + void testDeserializeWithEmptyStrings() throws JsonProcessingException + { + String json = "{\"compaction_type\":\"\",\"compaction_id\":\"\"}"; + CompactionStopRequestPayload payload = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(payload.compactionType()).isNull(); + assertThat(payload.compactionId()).isEmpty(); + } + + @Test + void testDeserializeWithWhitespace() throws JsonProcessingException + { + String json = "{\"compaction_type\":\" COMPACTION \",\"compaction_id\":\" test-id \"}"; + CompactionStopRequestPayload payload = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(payload.compactionType()).isEqualTo(CompactionType.COMPACTION); + assertThat(payload.compactionId()).isEqualTo(" test-id "); + } + + @Test + void testToString() + { + CompactionStopRequestPayload payload = new CompactionStopRequestPayload(CompactionType.COMPACTION, "abc-123"); + String toString = payload.toString(); + assertThat(toString).contains("compaction"); + assertThat(toString).contains("abc-123"); + assertThat(toString).contains("CompactionStopRequestPayload"); + } + + @Test + void testAllSupportedCompactionTypes() throws JsonProcessingException + { + + // Check each compactionType field for CompactionStopRequestPayload is recognized as a supported compaction type + for (CompactionType type : CompactionType.values()) { + CompactionStopRequestPayload payload = new CompactionStopRequestPayload(type, null); + String json = MAPPER.writeValueAsString(payload); + assertThat(json).contains(type.toString().toUpperCase()); + + CompactionStopRequestPayload deser = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(deser.compactionType()).isEqualTo(type); + } + } + + @Test + void testCasePreservation() throws JsonProcessingException + { + // Test preserved in serialization/deserialization + CompactionStopRequestPayload lowerCase = new CompactionStopRequestPayload(CompactionType.COMPACTION, "Test-ID-123"); + String json = MAPPER.writeValueAsString(lowerCase); + assertThat(json).contains("compaction"); + assertThat(json).contains("Test-ID-123"); + + CompactionStopRequestPayload deser = MAPPER.readValue(json, CompactionStopRequestPayload.class); + assertThat(deser.compactionType()).isEqualTo(CompactionType.COMPACTION); + assertThat(deser.compactionId()).isEqualTo("Test-ID-123"); + } +} Review Comment: Please add a new line at the end of the file ########## adapters/adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java: ########## @@ -53,4 +55,31 @@ public List<Map<String, String>> getCompactions() return jmxClient.proxy(CompactionManagerJmxOperations.class, COMPACTION_MANAGER_OBJ_NAME) .getCompactions(); } + + /** + * {@inheritDoc} + */ + @Override + public void stopCompactionById(String compactionId) + { + // compactionId takes precedence over type if both are provided + if (compactionId != null && !compactionId.trim().isEmpty()) { + CompactionManagerJmxOperations proxy = jmxClient.proxy( + CompactionManagerJmxOperations.class, + COMPACTION_MANAGER_OBJ_NAME + ); + proxy.stopCompactionById(compactionId); + } Review Comment: Do you want to throw an exception when the compactionId is invalid here and in the `stopCompaction` method ########## server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java: ########## @@ -0,0 +1,168 @@ +/* + * 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.handlers; + +import java.util.Collections; +import java.util.Set; + +import com.google.inject.Inject; +import io.netty.handler.codec.http.HttpResponseStatus; +import io.vertx.core.http.HttpServerRequest; +import io.vertx.core.json.DecodeException; +import io.vertx.core.json.Json; +import io.vertx.core.net.SocketAddress; +import io.vertx.ext.auth.authorization.Authorization; +import io.vertx.ext.web.RoutingContext; +import org.apache.cassandra.sidecar.acl.authorization.BasicPermissions; +import org.apache.cassandra.sidecar.common.data.CompactionStopStatus; +import org.apache.cassandra.sidecar.common.data.CompactionType; +import org.apache.cassandra.sidecar.common.request.data.CompactionStopRequestPayload; +import org.apache.cassandra.sidecar.common.response.CompactionStopResponse; +import org.apache.cassandra.sidecar.common.server.CompactionManagerOperations; +import org.apache.cassandra.sidecar.concurrent.ExecutorPools; +import org.apache.cassandra.sidecar.utils.CassandraInputValidator; +import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher; +import org.jetbrains.annotations.NotNull; + +import static org.apache.cassandra.sidecar.utils.HttpExceptions.wrapHttpException; + +/** + * Handler for stopping compaction operations via the Cassandra Compaction Manager. + * + * <p>Handles {@code POST /api/v1/cassandra/operations/compaction/stop} requests to stop + * compaction operations. Expects a JSON payload with compaction_type and/or compaction_id: + * <pre> + * { "compaction_type": "COMPACTION", "compaction_id": "abc-123" } + * </pre> + */ +public class CompactionStopHandler extends AbstractHandler<CompactionStopRequestPayload> implements AccessProtected +{ + /** + * Constructs a handler with the provided {@code metadataFetcher} + * + * @param metadataFetcher the metadata fetcher + * @param executorPools executor pools for blocking executions + * @param validator validator for Cassandra-specific input + */ + @Inject + protected CompactionStopHandler(final InstanceMetadataFetcher metadataFetcher, + final ExecutorPools executorPools, + final CassandraInputValidator validator) { + super(metadataFetcher, executorPools, validator); + } + + @Override + public Set<Authorization> requiredAuthorizations() { + return Collections.singleton(BasicPermissions.MODIFY_COMPACTION.toAuthorization()); + } + + /** + * {@inheritDoc} + */ + @Override + public void handleInternal(RoutingContext context, + HttpServerRequest httpRequest, + @NotNull String host, + SocketAddress remoteAddress, + CompactionStopRequestPayload request) { + CompactionManagerOperations compactionManagerOps = metadataFetcher.delegate(host).compactionManagerOperations(); + + executorPools.service() + .executeBlocking(() -> stopCompaction(compactionManagerOps, request)) + .onSuccess(context::json) + .onFailure(cause -> processFailure(cause, context, host, remoteAddress, request)); + } + + /** + * Stops the compaction based on the request parameters + * + * @param operations the compaction manager operations + * @param request the request payload containing compaction_type and/or compaction_id + * @return CompactionStopResponse with the operation result + */ + private CompactionStopResponse stopCompaction(CompactionManagerOperations operations, + CompactionStopRequestPayload request) + { + CompactionType compactionType = request.compactionType(); + String compactionId = request.compactionId(); + + // Convert enum to string for the operation (if not null) + // Use .name() to get uppercase enum name, matching Cassandra's OperationType enum format + String compactionTypeStr = compactionType != null ? compactionType.name() : null; + + // Attempt to stop the compaction + // If compactionId is provided, use it (takes precedence over type) + if (request.hasValidCompactionId()) { + operations.stopCompactionById(compactionId); + } else if (request.hasValidCompactionType()) { + operations.stopCompaction(compactionTypeStr); + } + // If we reach here, at least one of the above conditions was true due to validation in extractParamsOrThrow() + // Return success response + return CompactionStopResponse.builder() + .compactionType(compactionType) + .compactionId(compactionId) + .status(CompactionStopStatus.SUBMITTED) + .build(); + } + + /** + * Override extractParamsOrThrow to support compactionStop param constraints + * Method extracts and validates compaction stop request from routing context + * + * @param context the routing context + * @return validated CompactionStopRequestPayload + */ + @Override + protected CompactionStopRequestPayload extractParamsOrThrow(RoutingContext context) + { + String body = context.body().asString(); + CompactionStopRequestPayload payload; + + /* + Return 400 BAD_REQUEST upon malformed JSON - avoids falling under processFailure()'s vague + 500 INTERNAL_SERVER_ERROR. Also catches invalid compaction types via CompactionType.fromString() + */ + try + { + payload = Json.decodeValue(body, CompactionStopRequestPayload.class); + } + catch (DecodeException e) + { + logger.warn("Bad request. Received invalid JSON payload: {}", e.getMessage()); + throw wrapHttpException(HttpResponseStatus.BAD_REQUEST, + "Invalid JSON payload: " + e.getMessage()); + } + catch (IllegalArgumentException e) + { + logger.warn("Bad request. {}", e.getMessage()); + throw wrapHttpException(HttpResponseStatus.BAD_REQUEST, e.getMessage()); + } + + // Validate that at least one field is provided + if (!payload.atLeastOneParamProvided()) + { + logger.warn("Bad request. Both compaction_type and compaction_id are missing."); + throw wrapHttpException(HttpResponseStatus.BAD_REQUEST, + "At least one of 'compaction_type' or 'compaction_id' must be provided"); + } + + return payload; + } +} Review Comment: Please add a new line here -- 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]
