blerer commented on code in PR #2807: URL: https://github.com/apache/cassandra/pull/2807#discussion_r1374357211
########## src/java/org/apache/cassandra/tcm/transformations/AlterSchema.java: ########## @@ -0,0 +1,280 @@ +/* + * 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.tcm.transformations; + +import java.io.IOException; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.apache.cassandra.exceptions.AlreadyExistsException; +import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.exceptions.InvalidRequestException; +import org.apache.cassandra.exceptions.SyntaxException; +import org.apache.cassandra.io.util.DataInputPlus; +import org.apache.cassandra.io.util.DataOutputPlus; +import org.apache.cassandra.schema.DistributedSchema; +import org.apache.cassandra.schema.KeyspaceMetadata; +import org.apache.cassandra.schema.Keyspaces; +import org.apache.cassandra.schema.ReplicationParams; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.SchemaProvider; +import org.apache.cassandra.schema.SchemaTransformation; +import org.apache.cassandra.schema.TableMetadata; +import org.apache.cassandra.schema.Tables; +import org.apache.cassandra.tcm.ClusterMetadata; +import org.apache.cassandra.tcm.ClusterMetadataService; +import org.apache.cassandra.tcm.Epoch; +import org.apache.cassandra.tcm.Transformation; +import org.apache.cassandra.tcm.ownership.DataPlacement; +import org.apache.cassandra.tcm.ownership.DataPlacements; +import org.apache.cassandra.tcm.sequences.LockedRanges; +import org.apache.cassandra.tcm.serialization.AsymmetricMetadataSerializer; +import org.apache.cassandra.tcm.serialization.Version; +import org.apache.cassandra.utils.JVMStabilityInspector; + +import static org.apache.cassandra.exceptions.ExceptionCode.ALREADY_EXISTS; +import static org.apache.cassandra.exceptions.ExceptionCode.CONFIG_ERROR; +import static org.apache.cassandra.exceptions.ExceptionCode.INVALID; +import static org.apache.cassandra.exceptions.ExceptionCode.SERVER_ERROR; +import static org.apache.cassandra.exceptions.ExceptionCode.SYNTAX_ERROR; + +public class AlterSchema implements Transformation +{ + private static final Logger logger = LoggerFactory.getLogger(AlterSchema.class); + public static final Serializer serializer = new Serializer(); + + public final SchemaTransformation schemaTransformation; + protected final SchemaProvider schemaProvider; + + public AlterSchema(SchemaTransformation schemaTransformation, + SchemaProvider schemaProvider) + { + this.schemaTransformation = schemaTransformation; + this.schemaProvider = schemaProvider; + } + + @Override + public Kind kind() + { + return Kind.SCHEMA_CHANGE; + } + + @Override + public final Result execute(ClusterMetadata prev) + { + Keyspaces newKeyspaces; + try + { + // Applying the schema transformation may produce client warnings. If this is being executed by a follower + // of the cluster metadata log, there is no client or ClientState, so warning collection is a no-op. + // When a DDL statement is received from an actual client, the transformation is checked for validation + // and warnings are captured at that point, before being submitted to the CMS. + // If the coordinator is a CMS member, then this method will be called as part of committing to the metadata + // log. In this case, there is a connected client and associated ClientState, so to avoid duplicate warnings + // pause capture and resume after in applying the schema change. + schemaTransformation.enterExecution(); + + // Guard against an invalid SchemaTransformation supplying a TableMetadata with a future epoch + newKeyspaces = schemaTransformation.apply(prev); + newKeyspaces.forEach(ksm -> { + ksm.tables.forEach(tm -> { + if (tm.epoch.isAfter(prev.nextEpoch())) + throw new InvalidRequestException(String.format("Invalid schema transformation. " + + "Resultant epoch for table metadata of %s.%s (%d) " + + "is greater than for cluster metadata (%d)", + ksm.name, tm.name, tm.epoch.getEpoch(), + prev.nextEpoch().getEpoch())); Review Comment: `InvalidRequestException` has always been used for error that were linked to a user request that was trying to do something that C* was not supporting (e.g. ordering by non-clustering columns). In the current patch, there are multiple places where we use that exception for errors on which the user has no control (e.g. some race conditions or internal errors). I believe that we need to change that because it can be really confusing from the user point of view. -- 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]
