yifan-c commented on code in PR #212: URL: https://github.com/apache/cassandra-sidecar/pull/212#discussion_r2115129344
########## client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java: ########## @@ -141,6 +141,27 @@ public final class ApiEndpointsV1 public static final String STREAM_STATS_ROUTE = API_V1 + CASSANDRA + "/stats/streams"; public static final String TABLE_STATS_ROUTE = API_V1 + CASSANDRA + PER_KEYSPACE + PER_TABLE + "/stats"; + + // Live Migration APIs + public static final String LIVE_MIGRATION_API_PREFIX = API_V1 + "/live-migration"; + + public static final String LIVE_MIGRATION_LIST_FILES_PATH = "/files-list"; Review Comment: To follow the REST api pattern in the project, the endpoint should be `GET /api/v1/.../files` instead of `/files-list`. ########## conf/sidecar.yaml: ########## @@ -362,3 +362,10 @@ s3_client: # uri: # username: # password: + +live_migration: + special_files_to_exclude: [] + special_dirs_to_exclude: + - ${DATA_FILE_DIR}/*/*/snapshots # Excludes snapshot directories to copy to destination + migration_map: + localhost1: localhost4 Review Comment: can you add document to explain what this map expects? ########## server/src/main/java/org/apache/cassandra/sidecar/livemigration/CassandraInstanceFiles.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.livemigration; + +import java.io.IOException; +import java.util.List; + +import org.apache.cassandra.sidecar.common.response.InstanceFileInfo; +import org.apache.cassandra.sidecar.config.LiveMigrationConfiguration; + +/** + * Lists files that a Cassandra instance has. + */ +public interface CassandraInstanceFiles +{ + /** + * Lists files belong to a Cassandra instance after excluding default set files to exclude + * {@link LiveMigrationConfiguration#filesToExclude()} and default set of directories to exclude + * {@link LiveMigrationConfiguration#directoriesToExclude()}. + * + * @return list of files a Cassandra instance has after excluding default files and folders. + * @throws IOException when failed to read any file or folder. + */ + List<InstanceFileInfo> getFiles() throws IOException; Review Comment: Depending on the FS, the number of files could exceed what `List` can hold, which is at most `int.MAX` number of elements. This extreme case is not likely to happen, but it would be good to consider it when designing the interface. For example, document that if there are exceeding number of files, it would truncate to at most `int.MAX`. Or, the method can return `Iterator` instead. Taking another look at the "list files" handler, since it is likely to have a lot of files on disk, I am wondering how large the response do we expect? Would it cause OOM or memory pressure? ########## server/src/main/java/org/apache/cassandra/sidecar/handlers/livemigration/LiveMigrationMap.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.livemigration; + +import java.util.Map; + +import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata; +import org.jetbrains.annotations.NotNull; + +/** + * Map of source to destination C* instances to do the Live Migration. + */ +public interface LiveMigrationMap +{ + /** + * Tells whether given instance is configured as source or not. + * + * @param instanceMeta Cassandra instance metadata + * @return true if given instance is configured as source for live migration. + */ + default boolean isSource(@NotNull final InstanceMetadata instanceMeta) Review Comment: nit: please remove `final` ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/InstanceFileInfo.java: ########## @@ -0,0 +1,78 @@ +/* + * 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 java.util.Objects; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Class for holding information of a downloadable file during Live Migration. + */ +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonInclude(JsonInclude.Include.NON_NULL) +public class InstanceFileInfo +{ + public final String fileUrl; + // size is only relevant when the fileType is 'FILE' + public final long size; + public final FileType fileType; + + // The latest timestamp at which the file/dir was modified represented in milliseconds. + public final long lastModifiedTime; + + public InstanceFileInfo(@JsonProperty("fileUrl") final String fileUrl, + @JsonProperty("size") final long size, + @JsonProperty("fileType") final FileType fileType, + @JsonProperty("lastModifiedTime") final long lastModifiedTime) Review Comment: nit: remove the unnecessary `final` modifiers ########## conf/sidecar.yaml: ########## @@ -362,3 +362,10 @@ s3_client: # uri: # username: # password: + +live_migration: + special_files_to_exclude: [] + special_dirs_to_exclude: Review Comment: nit: "special" can be removed, as it does not add context to the setting names. `files_to_exclude` means the same as `special_files_to_exclude` ########## server/src/main/java/org/apache/cassandra/sidecar/config/LiveMigrationConfiguration.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.config; + +import java.util.Map; +import java.util.Set; + +/** + * Configuration for Live Migration feature. + */ +public interface LiveMigrationConfiguration +{ + + /** + * Files to be excluded from Live Migration. + * @return set of file exclusion patterns. + */ + Set<String> filesToExclude(); + + /** + * Directories to be excluded from Live Migration. + * @return set of directory exclusion patterns. + */ + Set<String> directoriesToExclude(); + + /** + * Map of source and destination Cassandra instances to migrate. + * + * @return Map of strings where key is the source instance name and value is the destination instance name. Review Comment: Do you mean `instance hostname` instead of `instance name`? ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/InstanceFileInfo.java: ########## @@ -0,0 +1,78 @@ +/* + * 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 java.util.Objects; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Class for holding information of a downloadable file during Live Migration. + */ +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonInclude(JsonInclude.Include.NON_NULL) +public class InstanceFileInfo +{ + public final String fileUrl; + // size is only relevant when the fileType is 'FILE' + public final long size; Review Comment: nit: can you aslo document the value of `size` when the `fileType` is `DIRECTORY`? ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/InstanceFilesListResponse.java: ########## @@ -0,0 +1,106 @@ +/* + * 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 java.util.List; +import java.util.Objects; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.cassandra.sidecar.common.ApiEndpointsV1; +import org.jetbrains.annotations.NotNull; + +/** + * Class representing response for {@link ApiEndpointsV1#LIVE_MIGRATION_LIST_FILES_API}. + * Holds list of file urls (with their individual sizes) + total size of files + * <p> + * Sample format: + * <pre> + * { + * "totalSize": 123456789, + * "files" : [ + * { + * "fileUrl": "/api/v1/live-migration/files/{type_of_directory}/{directory_index}/{relative_file_path}", + * "size": 123456789, + * "fileType": "FILE", + * "lastModifiedTime": 1707193655406 + * }, + * + * { + * "fileUrl": "/api/v1/live-migration/files/{type_of_directory}/{directory_index}/{relative_file_path}", + * "size": -1, + * "fileType": "DIRECTORY", + * "lastModifiedTime": 1707798915999 + * } + * ] + * } + * </pre> + */ +public class InstanceFilesListResponse +{ + private final List<InstanceFileInfo> files; + private final long totalSize; + + public InstanceFilesListResponse(@NotNull final List<InstanceFileInfo> files) + { + this.files = files; + long sum = 0L; + for (InstanceFileInfo file : files) + { + if (file.fileType.equals(InstanceFileInfo.FileType.FILE)) + { + long size = file.size; + sum += size; + } + } + totalSize = sum; + } + + @JsonCreator + public InstanceFilesListResponse(@JsonProperty("files") final List<InstanceFileInfo> files, + @JsonProperty("totalSize") final long totalSize) Review Comment: nit: remove the `final`s for the arguments. ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/InstanceFilesListResponse.java: ########## @@ -0,0 +1,106 @@ +/* + * 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 java.util.List; +import java.util.Objects; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.cassandra.sidecar.common.ApiEndpointsV1; +import org.jetbrains.annotations.NotNull; + +/** + * Class representing response for {@link ApiEndpointsV1#LIVE_MIGRATION_LIST_FILES_API}. + * Holds list of file urls (with their individual sizes) + total size of files + * <p> + * Sample format: + * <pre> + * { + * "totalSize": 123456789, + * "files" : [ + * { + * "fileUrl": "/api/v1/live-migration/files/{type_of_directory}/{directory_index}/{relative_file_path}", + * "size": 123456789, + * "fileType": "FILE", + * "lastModifiedTime": 1707193655406 + * }, + * + * { + * "fileUrl": "/api/v1/live-migration/files/{type_of_directory}/{directory_index}/{relative_file_path}", + * "size": -1, + * "fileType": "DIRECTORY", + * "lastModifiedTime": 1707798915999 + * } + * ] + * } + * </pre> + */ +public class InstanceFilesListResponse Review Comment: 👍 ########## server/src/main/java/org/apache/cassandra/sidecar/config/LiveMigrationConfiguration.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.config; + +import java.util.Map; +import java.util.Set; + +/** + * Configuration for Live Migration feature. + */ +public interface LiveMigrationConfiguration +{ + + /** + * Files to be excluded from Live Migration. + * @return set of file exclusion patterns. + */ + Set<String> filesToExclude(); + + /** + * Directories to be excluded from Live Migration. + * @return set of directory exclusion patterns. + */ + Set<String> directoriesToExclude(); + + /** + * Map of source and destination Cassandra instances to migrate. + * + * @return Map of strings where key is the source instance name and value is the destination instance name. + */ + Map<String, String> migrationMap(); +} Review Comment: It is not covered in CEP-40, but reading this patch makes me concerning about the operation complexity. If the live migration configuration is expressed in the form of yaml config, it would require rolling restart of sidecar to pick up the configurations. If my understanding is correct, live migration is a task, the task configuration should be per task, not per cluster (when it is stored in sidecar.yaml). If you agree with me, a more suitable place for storing the task configuration is in a table in sidecar internal keyspace. ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/InstanceFilesListResponse.java: ########## @@ -0,0 +1,106 @@ +/* + * 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 java.util.List; +import java.util.Objects; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.cassandra.sidecar.common.ApiEndpointsV1; +import org.jetbrains.annotations.NotNull; + +/** + * Class representing response for {@link ApiEndpointsV1#LIVE_MIGRATION_LIST_FILES_API}. + * Holds list of file urls (with their individual sizes) + total size of files + * <p> + * Sample format: + * <pre> + * { + * "totalSize": 123456789, + * "files" : [ + * { + * "fileUrl": "/api/v1/live-migration/files/{type_of_directory}/{directory_index}/{relative_file_path}", + * "size": 123456789, + * "fileType": "FILE", + * "lastModifiedTime": 1707193655406 + * }, + * + * { + * "fileUrl": "/api/v1/live-migration/files/{type_of_directory}/{directory_index}/{relative_file_path}", + * "size": -1, + * "fileType": "DIRECTORY", + * "lastModifiedTime": 1707798915999 + * } + * ] + * } + * </pre> + */ +public class InstanceFilesListResponse +{ + private final List<InstanceFileInfo> files; Review Comment: nit: consider making an immutable copy for the list. ########## server/src/main/java/org/apache/cassandra/sidecar/handlers/livemigration/LiveMigrationMap.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.livemigration; + +import java.util.Map; + +import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata; +import org.jetbrains.annotations.NotNull; + +/** + * Map of source to destination C* instances to do the Live Migration. + */ +public interface LiveMigrationMap +{ + /** + * Tells whether given instance is configured as source or not. + * + * @param instanceMeta Cassandra instance metadata + * @return true if given instance is configured as source for live migration. + */ + default boolean isSource(@NotNull final InstanceMetadata instanceMeta) + { + Map<String, String> map = getMigrationMap(); + return map != null && map.containsKey(String.valueOf(instanceMeta.host())); + } + + /** + * Tells whether given instance is configured as destination or not. + * + * @param instanceMeta Cassandra instance metadata + * @return true if given instance is configured as destination for live migration. + */ + default boolean isDestination(@NotNull final InstanceMetadata instanceMeta) + { + Map<String, String> map = getMigrationMap(); + return map != null && map.containsValue(String.valueOf(instanceMeta.host())); + } + + /** + * Tells whether given instance is configured as either source or destination. + * + * @param instanceMeta Cassandra instance metadata + * @return true if given instance is either source or destination + */ + default boolean isAny(@NotNull final InstanceMetadata instanceMeta) + { + return isSource(instanceMeta) || isDestination(instanceMeta); + } + + /** + * Returns map of source and destination + * + * @return map of source and destination + */ + Map<String, String> getMigrationMap(); Review Comment: can this ever return null? if so, please annotate `@Nullable`. I would suggest avoid `null` values at the best, and _optionally_ annotate `@NotNull`. It would simplify the callsites, e.g. `isDestination` ########## server/src/main/java/org/apache/cassandra/sidecar/config/yaml/LiveMigrationConfigurationImpl.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.config.yaml; + +import java.util.Collections; +import java.util.Map; +import java.util.Set; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.cassandra.sidecar.config.LiveMigrationConfiguration; + +/** + * Configuration required for live migrating the Cassandra instances. + */ +public class LiveMigrationConfigurationImpl implements LiveMigrationConfiguration +{ + + private final Set<String> filesToExclude; + private final Set<String> directoriesToExclude; + private final Map<String, String> migrationMap; + + public LiveMigrationConfigurationImpl() + { + this(Collections.emptySet(), Collections.emptySet(), Collections.emptyMap()); + } + + @JsonCreator + public LiveMigrationConfigurationImpl(@JsonProperty("files_to_exclude") Set<String> filesToExclude, + @JsonProperty("dirs_to_exclude") Set<String> directoriesToExclude, + @JsonProperty("migration_map") Map<String, String> migrationMap) Review Comment: The code refers to the fields as `files_to_exclude` and `dirs_to_exclude` already. Is there a test to check the yaml file? Feel like not, otherwise, the test would have failed. That being said, I lean towards persisting the configuration for live migration tasks in a table. -- 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