Copilot commented on code in PR #3256: URL: https://github.com/apache/fluss/pull/3256#discussion_r3206862818
########## fluss-lake/fluss-lake-hudi/pom.xml: ########## @@ -0,0 +1,57 @@ +<?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.fluss</groupId> + <artifactId>fluss-lake</artifactId> + <version>0.9-SNAPSHOT</version> Review Comment: The parent version is set to 0.9-SNAPSHOT, but the rest of the project (and the fluss-lake parent) is 1.0-SNAPSHOT. This will break the reactor build / parent resolution; the module should inherit the same version as the current Fluss build. ########## fluss-common/src/main/java/org/apache/fluss/metadata/DataLakeFormat.java: ########## @@ -21,7 +21,8 @@ public enum DataLakeFormat { PAIMON("paimon"), LANCE("lance"), - ICEBERG("iceberg"); + ICEBERG("iceberg"), + HUDI("hudi"); Review Comment: Adding HUDI to DataLakeFormat makes it a valid config value, but several core factories (e.g., bucketing/key encode/decode) currently throw UnsupportedOperationException for any lake format other than PAIMON/LANCE/ICEBERG. If HUDI is intended to be selectable now, those call sites need to handle HUDI (even if by mapping to an existing implementation); otherwise consider deferring the enum addition or failing earlier with a clearer validation error. ########## fluss-lake/fluss-lake-hudi/pom.xml: ########## @@ -0,0 +1,57 @@ +<?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.fluss</groupId> + <artifactId>fluss-lake</artifactId> + <version>0.9-SNAPSHOT</version> + </parent> + + <artifactId>fluss-lake-hudi</artifactId> + <name>Fluss : Lake : Hudi</name> + + <packaging>jar</packaging> + + + <dependencies> + <dependency> + <groupId>org.apache.fluss</groupId> + <artifactId>fluss-common</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + <exclusions> + <exclusion> + <groupId>log4j</groupId> + <artifactId>log4j</artifactId> + </exclusion> + <exclusion> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-api</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.commons</groupId> + <artifactId>commons-lang3</artifactId> + </exclusion> + </exclusions> Review Comment: This module declares fluss-common with scope 'compile' and adds exclusions, while other lake format modules use scope 'provided' for fluss-common. Consider aligning to 'provided' here as well (and dropping unnecessary exclusions) so consumers/distribution don't accidentally pull in Fluss internals transitively. ########## fluss-lake/fluss-lake-hudi/src/main/java/org/apache/fluss/lake/hudi/HudiLakeStoragePlugin.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.fluss.lake.hudi; + +import org.apache.fluss.config.Configuration; +import org.apache.fluss.lake.lakestorage.LakeStorage; +import org.apache.fluss.lake.lakestorage.LakeStoragePlugin; + +/** Implement of Hudi Lake Storage Plugin. */ Review Comment: Javadoc grammar is off/inconsistent with other LakeStoragePlugin implementations ("Implement of ..."). Consider changing to "Hudi implementation of {@link LakeStoragePlugin}" (or similar) for clarity and consistency. ########## fluss-lake/fluss-lake-hudi/src/test/resources/log4j2-test.properties: ########## @@ -0,0 +1,26 @@ +# +# 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. +# +# Set root logger level to OFF to not flood build logs +# set manually to INFO for debugging purposes +rootLogger.level=INFO Review Comment: The comment says the root logger level should be OFF to avoid flooding build logs, but it is set to INFO. This is inconsistent with other modules’ log4j2-test.properties and will likely increase test output noise; consider setting it back to OFF. ########## fluss-lake/fluss-lake-hudi/src/main/java/org/apache/fluss/lake/hudi/HudiLakeStorage.java: ########## @@ -0,0 +1,50 @@ +/* + * 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.fluss.lake.hudi; + +import org.apache.fluss.config.Configuration; +import org.apache.fluss.lake.lakestorage.LakeCatalog; +import org.apache.fluss.lake.lakestorage.LakeStorage; +import org.apache.fluss.lake.source.LakeSource; +import org.apache.fluss.lake.writer.LakeTieringFactory; +import org.apache.fluss.metadata.TablePath; + +/** Implement of Hudi Lake Source. */ Review Comment: Class Javadoc says "Implement of Hudi Lake Source" but this class implements LakeStorage, not LakeSource. Please fix the description (and grammar) to avoid confusing API readers. ########## fluss-lake/fluss-lake-hudi/src/main/resources/META-INF/NOTICE: ########## @@ -0,0 +1,9 @@ +fluss-lake-hudi +Copyright 2025-2026 The Apache Software Foundation + +This project includes software developed at +The Apache Software Foundation (http://www.apache.org/). + +This project bundles the following dependencies under the Apache Software License 2.0 (http://www.apache.org/licenses/LICENSE-2.0.txt) + +- org.apache.hudi:hudi-flink1.20-bundle:1.1.0 Review Comment: NOTICE states that the module bundles org.apache.hudi:hudi-flink1.20-bundle:1.1.0, but the module POM and the distribution plugin assembly don’t include that artifact in the produced plugin directory. Please either actually bundle/copy the dependency into the plugin distribution or remove/correct the NOTICE entry to match what is shipped. ########## fluss-lake/fluss-lake-hudi/src/main/java/org/apache/fluss/lake/hudi/HudiLakeStorage.java: ########## @@ -0,0 +1,50 @@ +/* + * 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.fluss.lake.hudi; + +import org.apache.fluss.config.Configuration; +import org.apache.fluss.lake.lakestorage.LakeCatalog; +import org.apache.fluss.lake.lakestorage.LakeStorage; +import org.apache.fluss.lake.source.LakeSource; +import org.apache.fluss.lake.writer.LakeTieringFactory; +import org.apache.fluss.metadata.TablePath; + +/** Implement of Hudi Lake Source. */ +public class HudiLakeStorage implements LakeStorage { + + protected final Configuration hudiConfig; + + public HudiLakeStorage(Configuration configuration) { + this.hudiConfig = configuration; + } + + @Override + public LakeTieringFactory<?, ?> createLakeTieringFactory() { + throw new UnsupportedOperationException("Not implemented"); + } + + @Override + public LakeCatalog createLakeCatalog() { + throw new UnsupportedOperationException("Not implemented"); + } + + @Override + public LakeSource<?> createLakeSource(TablePath tablePath) { + throw new UnsupportedOperationException("Not implemented"); Review Comment: These stubs throw UnsupportedOperationException("Not implemented"), which will surface to users if HUDI is selected and make failures hard to diagnose. Prefer a more actionable message (e.g., stating that HudiLakeStorage is a scaffold/not supported yet and pointing to required config/module) so misconfiguration is clearer. ########## fluss-flink/fluss-flink-common/pom.xml: ########## @@ -95,6 +95,24 @@ <scope>provided</scope> </dependency> + <!-- Hudi --> + <dependency> + <groupId>org.apache.hudi</groupId> + <artifactId>hudi-flink${flink.major.version}-bundle</artifactId> + <version>${hudi.version}</version> + <scope>provided</scope> + <exclusions> + <exclusion> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-reload4j</artifactId> + </exclusion> + <exclusion> + <groupId>ch.qos.reload4j</groupId> + <artifactId>reload4j</artifactId> + </exclusion> + </exclusions> + </dependency> + Review Comment: The fluss-flink-common module adds a provided dependency on the Hudi Flink bundle, but there are no references to org.apache.hudi in this module’s sources. Carrying this dependency here increases build/dependency surface area for all Flink integrations; consider moving it to the new fluss-lake-hudi module (and/or the dist plugin assembly) where it’s actually needed. -- 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]
