jyothsnakonisa commented on code in PR #193: URL: https://github.com/apache/cassandra-sidecar/pull/193#discussion_r1964369418
########## server/src/main/java/org/apache/cassandra/sidecar/routes/cdc/UpdateCdcConfigRequestValidationHandler.java: ########## @@ -0,0 +1,44 @@ +/* + * 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.cdc; + +import com.google.inject.Singleton; +import io.vertx.core.Handler; +import io.vertx.core.json.JsonObject; +import io.vertx.ext.web.RoutingContext; + + +/** + * Updating a service config from "configs" table in sidecar keyspace should perform some + * validation checks before updating the config. + * {@link DeleteCdcConfigRequestValidationHandler} performs those validations before the + * update operation. + */ +@Singleton +public class UpdateCdcConfigRequestValidationHandler implements Handler<RoutingContext> +{ + @Override + public void handle(RoutingContext context) + { + final JsonObject payload = context.getBodyAsJson(); + ServiceConfigValidators.verifyValidPayload(context, payload); + ServiceConfigValidators.verifyValidService(context, payload); + ServiceConfigValidators.verifyValidConfig(context, payload); + context.next(); + } +} Review Comment: Added `requiredAuthorizations` ########## server/src/main/java/org/apache/cassandra/sidecar/config/yaml/CdcConfigurationImpl.java: ########## @@ -32,22 +33,50 @@ public class CdcConfigurationImpl implements CdcConfiguration { private static final Logger LOGGER = LoggerFactory.getLogger(CdcConfigurationImpl.class); + public static final String IS_ENABLED_PROPERTY = "is_enabled"; + public static final String CONFIGURATION_REFRESH_TIME_PROPERTY = "config_refresh_time"; public static final String SEGMENT_HARD_LINK_CACHE_EXPIRY_PROPERTY = "segment_hardlink_cache_expiry"; + public static final boolean DEFAULT_IS_ENABLED = false; + public static final MillisecondBoundConfiguration DEFAULT_CDC_CONFIG_REFRESH_TIME = + MillisecondBoundConfiguration.parse("30s"); public static final SecondBoundConfiguration DEFAULT_SEGMENT_HARD_LINK_CACHE_EXPIRY = - SecondBoundConfiguration.parse("5m"); + SecondBoundConfiguration.parse("5m"); + protected boolean isEnabled; + protected MillisecondBoundConfiguration cdcConfigRefreshTime; protected SecondBoundConfiguration segmentHardLinkCacheExpiry; + public CdcConfigurationImpl() { this.segmentHardLinkCacheExpiry = DEFAULT_SEGMENT_HARD_LINK_CACHE_EXPIRY; + this.cdcConfigRefreshTime = DEFAULT_CDC_CONFIG_REFRESH_TIME; + this.isEnabled = DEFAULT_IS_ENABLED; } - public CdcConfigurationImpl(SecondBoundConfiguration segmentHardLinkCacheExpiry) + public CdcConfigurationImpl(boolean isEnabled, + MillisecondBoundConfiguration cdcConfigRefreshTime, + SecondBoundConfiguration segmentHardLinkCacheExpiry) Review Comment: `@JsonProperty` annotations are added in the getter and setter methods below but I think it might be clean if we add annotation to properties instead. Also, there is already a test that validates deserializing from config.yaml in `SidecarConfigurationTest.testCdcConfiguration` ########## server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.db; + +import com.google.inject.Inject; +import org.apache.cassandra.sidecar.routes.cdc.ValidServices; + +/** + * Factory for creating config objects based on the service name. + */ +public class ConfigAccessorFactory +{ + private final KafkaConfigAccessor kafkaConfigAccessor; + private final CdcConfigAccessor cdcConfigAccessor; Review Comment: I prefer having two separate objects, which are explicit instead of putting just two objects in the map and getting them with a string key. ########## server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfig.java: ########## @@ -0,0 +1,181 @@ +/* + * 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.cdc; + +import java.time.Duration; +import java.util.Map; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +/** + * In memory representation of CDC and Kafka configurations from "configs" table in sidecar keyspace. + */ +public interface CdcConfig +{ + Logger LOGGER = LoggerFactory.getLogger(CdcConfig.class); + + String DEFAULT_JOB_ID = "test-job"; + int DEFAULT_MAX_WATERMARKER_SIZE = 400000; Review Comment: The constant is used in the interface for default value and relocation to implementation will now work in this case ########## server/src/main/java/org/apache/cassandra/sidecar/routes/cdc/DeleteServiceConfigHandler.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.cdc; + + +import java.util.Collections; +import java.util.NoSuchElementException; +import java.util.Set; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import io.netty.handler.codec.http.HttpResponseStatus; +import io.vertx.core.Handler; +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.db.ConfigAccessor; +import org.apache.cassandra.sidecar.db.ConfigAccessorFactory; +import org.apache.cassandra.sidecar.routes.AccessProtected; +import static org.apache.cassandra.sidecar.utils.HttpExceptions.wrapHttpException; + +/** + * Provides REST endpoint for deleting CDC/Kafka configs + */ +@Singleton +public class DeleteServiceConfigHandler implements Handler<RoutingContext>, AccessProtected +{ + private final ConfigAccessorFactory configAccessorFactory; + + @Inject + public DeleteServiceConfigHandler(final ConfigAccessorFactory configAccessorFactory) Review Comment: Should we make the class final?? I think all other handler classes are also not final classes. ########## server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorImpl.java: ########## @@ -0,0 +1,114 @@ +/* + * 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.db; + +import java.util.Map; +import java.util.Optional; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import com.datastax.driver.core.BoundStatement; +import com.datastax.driver.core.ResultSet; +import com.datastax.driver.core.Row; + +import org.apache.cassandra.sidecar.common.server.CQLSessionProvider; +import org.apache.cassandra.sidecar.db.schema.ConfigsSchema; +import org.apache.cassandra.sidecar.db.schema.SidecarSchema; +import org.apache.cassandra.sidecar.routes.cdc.ValidServices; +import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher; + +/** + * Configurations for CDC feature are stored inside a table "config" in an internal sidecar keyspace. + * {@link ConfigAccessorImpl} is an accessor for the above-mentioned table and encapsulates database + * access operations of the "config" table. + */ +public abstract class ConfigAccessorImpl extends DatabaseAccessor<ConfigsSchema> implements ConfigAccessor +{ + private static final Logger LOGGER = LoggerFactory.getLogger(ConfigAccessorImpl.class); + private final ValidServices service = service(); + private final SidecarSchema sidecarSchema; + + protected ConfigAccessorImpl(InstanceMetadataFetcher instanceMetadataFetcher, + ConfigsSchema configsSchema, + CQLSessionProvider sessionProvider, + SidecarSchema sidecarSchema) + { + super(configsSchema, sessionProvider); + this.sidecarSchema = sidecarSchema; + } + + public abstract ValidServices service(); + + @Override + public ServiceConfig getConfig() + { + sidecarSchema.ensureInitialized(); + BoundStatement statement = tableSchema + .selectConfig() + .bind(service.serviceName); + Row row = execute(statement).one(); + if (row == null || row.isNull(0)) + { + LOGGER.debug(String.format("No %s configs are present in the table C* table", service.serviceName)); + return new ServiceConfig(Map.of()); + } + return ServiceConfig.from(row); + } + + @Override + public ServiceConfig storeConfig(Map<String, String> config) + { + sidecarSchema.ensureInitialized(); + BoundStatement statement = tableSchema + .insertConfig() + .bind(service.serviceName, config); + execute(statement); + return new ServiceConfig(config); + } + + @Override + public Optional<ServiceConfig> storeConfigIfNotExists(Map<String, String> config) Review Comment: It is used in some classes that we will be adding in other patches after this patch. we can remove it for now and add it or else we can add it now. I don't have a strong preference ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java: ########## @@ -121,6 +121,12 @@ public final class ApiEndpointsV1 public static final String LIST_CDC_SEGMENTS_ROUTE = API_V1 + CDC_PATH + "/segments"; public static final String STREAM_CDC_SEGMENTS_ROUTE = LIST_CDC_SEGMENTS_ROUTE + "/" + SEGMENT_PATH_PARAM; + public static final String SERVICES_PATH = "/services"; + public static final String SERVICE_PARAM = ":service"; + public static final String CONFIG = "/config"; + public static final String SERVICE_CONFIG_ROUTE = API_V1 + SERVICES_PATH + SERVICE_PARAM + CONFIG; + public static final String GET_SERVICES_CONFIG_ROUTE = API_V1 + SERVICES_PATH; Review Comment: The get verb gets config for all the services, I think it is valuable. If we want to be able to get configs of a given service we can add GET verb on `/api/v1/config/services/:service`. -- 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: pr-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org For additional commands, e-mail: pr-h...@cassandra.apache.org
