TyrantLucifer commented on code in PR #9576: URL: https://github.com/apache/seatunnel/pull/9576#discussion_r2549602534
########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/sink/FlinkWriterState.java: ########## @@ -0,0 +1,54 @@ +/* Review Comment: Why not use org.apache.seatunnel.translation.flink.sink.FlinkWriterState that in common module? ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-20-starter/pom.xml: ########## @@ -0,0 +1,138 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter</artifactId> + <version>${revision}</version> + </parent> + + <artifactId>seatunnel-flink-20-starter</artifactId> + <name>SeaTunnel : Core : Flink Starter : 2.0</name> + + <properties> + <flink.scope>provided</flink.scope> + </properties> + + <dependencies> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter-common</artifactId> + <version>${project.version}</version> + </dependency> + + <!-- Flink 1.20 specific translation - higher priority --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-20</artifactId> + <version>${project.version}</version> + <scope>compile</scope> Review Comment: remove useless tag ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-20-starter/pom.xml: ########## @@ -0,0 +1,138 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter</artifactId> + <version>${revision}</version> + </parent> + + <artifactId>seatunnel-flink-20-starter</artifactId> + <name>SeaTunnel : Core : Flink Starter : 2.0</name> + + <properties> + <flink.scope>provided</flink.scope> + </properties> + + <dependencies> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter-common</artifactId> + <version>${project.version}</version> + </dependency> + + <!-- Flink 1.20 specific translation - higher priority --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-20</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <!-- Common translation - fallback for missing classes --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-common</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-java</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-streaming-java</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-table-api-java-bridge</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-statebackend-rocksdb</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>connector-common</artifactId> + <version>${project.version}</version> + </dependency> + + <dependency> Review Comment: why add this dependency? ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/metric/FlinkAccumulatorCounter.java: ########## @@ -0,0 +1,128 @@ +/* Review Comment: Why not use LongCounter ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-20-starter/pom.xml: ########## @@ -0,0 +1,138 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter</artifactId> + <version>${revision}</version> + </parent> + + <artifactId>seatunnel-flink-20-starter</artifactId> + <name>SeaTunnel : Core : Flink Starter : 2.0</name> + + <properties> + <flink.scope>provided</flink.scope> + </properties> + + <dependencies> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter-common</artifactId> + <version>${project.version}</version> + </dependency> + + <!-- Flink 1.20 specific translation - higher priority --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-20</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <!-- Common translation - fallback for missing classes --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-common</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-java</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-streaming-java</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-table-api-java-bridge</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-statebackend-rocksdb</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>connector-common</artifactId> + <version>${project.version}</version> + </dependency> + + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-transforms-v2</artifactId> + <version>${project.version}</version> + </dependency> + </dependencies> + + <build> Review Comment: why add shade plugin? ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-starter-common/src/main/java/org/apache/seatunnel/core/starter/flink/execution/AbstractFlinkExecution.java: ########## @@ -0,0 +1,233 @@ +/* + * 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.seatunnel.core.starter.flink.execution; + +import org.apache.seatunnel.shade.com.typesafe.config.Config; +import org.apache.seatunnel.shade.com.typesafe.config.ConfigUtil; +import org.apache.seatunnel.shade.com.typesafe.config.ConfigValueFactory; + +import org.apache.seatunnel.api.common.JobContext; +import org.apache.seatunnel.api.options.EnvCommonOptions; +import org.apache.seatunnel.common.Constants; +import org.apache.seatunnel.common.config.Common; +import org.apache.seatunnel.common.config.TypesafeConfigUtils; +import org.apache.seatunnel.common.constants.JobMode; +import org.apache.seatunnel.common.utils.SeaTunnelException; +import org.apache.seatunnel.core.starter.exception.TaskExecuteException; +import org.apache.seatunnel.core.starter.execution.PluginExecuteProcessor; +import org.apache.seatunnel.core.starter.execution.RuntimeEnvironment; +import org.apache.seatunnel.core.starter.execution.TaskExecution; +import org.apache.seatunnel.core.starter.flink.FlinkStarter; +import org.apache.seatunnel.translation.flink.metric.FlinkJobMetricsSummary; + +import org.apache.flink.api.common.JobExecutionResult; +import org.apache.flink.api.common.RuntimeExecutionMode; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** Abstract base class for Flink execution implementations. */ +public abstract class AbstractFlinkExecution implements TaskExecution { Review Comment: Why need abstract? It only has one class to extend this. org.apache.seatunnel.core.starter.flink.execution.FlinkExecution ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/metric/FlinkMetricContext.java: ########## @@ -0,0 +1,121 @@ +/* Review Comment: The same as below ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/serialization/CommitWrapperSerializer.java: ########## @@ -0,0 +1,59 @@ +/* Review Comment: Why do not use old serializer sink#getCommitInfoSerializer and add new CommitWrapperSerializer.java ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/serialization/FlinkWriterStateSerializer.java: ########## @@ -0,0 +1,75 @@ +/* Review Comment: Why not use org.apache.seatunnel.translation.flink.serialization.FlinkWriterStateSerializer in seatunnel-translation-flink-common ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/metric/FlinkJobMetricsSummary.java: ########## @@ -0,0 +1,181 @@ +/* Review Comment: the same as below ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/metric/FlinkCounter.java: ########## @@ -0,0 +1,72 @@ +/* Review Comment: the same as below ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-20-starter/pom.xml: ########## @@ -0,0 +1,138 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter</artifactId> + <version>${revision}</version> + </parent> + + <artifactId>seatunnel-flink-20-starter</artifactId> + <name>SeaTunnel : Core : Flink Starter : 2.0</name> + + <properties> + <flink.scope>provided</flink.scope> + </properties> + + <dependencies> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter-common</artifactId> + <version>${project.version}</version> + </dependency> + + <!-- Flink 1.20 specific translation - higher priority --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-20</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <!-- Common translation - fallback for missing classes --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-common</artifactId> + <version>${project.version}</version> + <scope>compile</scope> Review Comment: the same as below ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-20-starter/pom.xml: ########## @@ -0,0 +1,138 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter</artifactId> + <version>${revision}</version> + </parent> + + <artifactId>seatunnel-flink-20-starter</artifactId> + <name>SeaTunnel : Core : Flink Starter : 2.0</name> + + <properties> + <flink.scope>provided</flink.scope> + </properties> + + <dependencies> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-flink-starter-common</artifactId> + <version>${project.version}</version> + </dependency> + + <!-- Flink 1.20 specific translation - higher priority --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-20</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <!-- Common translation - fallback for missing classes --> + <dependency> + <groupId>org.apache.seatunnel</groupId> + <artifactId>seatunnel-translation-flink-common</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-java</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-streaming-java</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-table-api-java-bridge</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> + <groupId>org.apache.flink</groupId> + <artifactId>flink-statebackend-rocksdb</artifactId> + <version>${flink.1.20.1.version}</version> + <scope>${flink.scope}</scope> + </dependency> + + <dependency> Review Comment: connector-common has be contained by parent module, why duplicate add it? ########## seatunnel-core/seatunnel-flink-starter/seatunnel-flink-starter-common/src/main/java/org/apache/seatunnel/core/starter/flink/execution/FlinkExecution.java: ########## @@ -18,46 +18,15 @@ package org.apache.seatunnel.core.starter.flink.execution; import org.apache.seatunnel.shade.com.typesafe.config.Config; Review Comment: The same as above, why need abstract? ########## seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-20/src/main/java/org/apache/seatunnel/translation/flink/metric/FlinkMeter.java: ########## @@ -0,0 +1,62 @@ +/* Review Comment: Why rewrite this class? It same as org.apache.seatunnel.translation.flink.metric.FlinkMeter -- 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]
