Copilot commented on code in PR #2055: URL: https://github.com/apache/auron/pull/2055#discussion_r2871538395
########## auron-flink-extension/auron-flink-runtime/src/test/java/org/apache/auron/flink/configuration/FlinkAuronConfigurationTest.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.auron.flink.configuration; + +import static org.junit.jupiter.api.Assertions.*; + +import java.io.File; +import java.net.URL; +import java.util.HashMap; +import java.util.Map; +import org.apache.auron.configuration.AuronConfiguration; +import org.apache.auron.flink.testutils.CommonTestUtils; +import org.apache.flink.configuration.ConfigConstants; +import org.apache.flink.configuration.GlobalConfiguration; +import org.junit.jupiter.api.Test; + +/** + * This class is used to test the FlinkAuronConfiguration class. + */ +public class FlinkAuronConfigurationTest { + + @Test + public void testGetConfigFromFlinkConfig() { + URL flinkConfigFileUrl = + FlinkAuronConfigurationTest.class.getClassLoader().getResource(GlobalConfiguration.FLINK_CONF_FILENAME); + Map<String, String> env = new HashMap<>(System.getenv()); + env.put( + ConfigConstants.ENV_FLINK_CONF_DIR, + new File(flinkConfigFileUrl.getFile()).getParentFile().getAbsolutePath()); Review Comment: `getResource(GlobalConfiguration.FLINK_CONF_FILENAME)` can return null; as written, the test will NPE with a less-informative failure. Also `new File(flinkConfigFileUrl.getFile())` can mis-handle URL-encoded paths (spaces, unicode). Consider asserting the resource is present and converting via `toURI()`/`Paths` before deriving the parent directory. ########## auron-flink-extension/auron-flink-runtime/src/test/java/org/apache/auron/flink/configuration/FlinkAuronConfigurationTest.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.auron.flink.configuration; + +import static org.junit.jupiter.api.Assertions.*; + +import java.io.File; +import java.net.URL; +import java.util.HashMap; +import java.util.Map; +import org.apache.auron.configuration.AuronConfiguration; +import org.apache.auron.flink.testutils.CommonTestUtils; +import org.apache.flink.configuration.ConfigConstants; +import org.apache.flink.configuration.GlobalConfiguration; +import org.junit.jupiter.api.Test; + +/** + * This class is used to test the FlinkAuronConfiguration class. + */ +public class FlinkAuronConfigurationTest { + + @Test + public void testGetConfigFromFlinkConfig() { + URL flinkConfigFileUrl = + FlinkAuronConfigurationTest.class.getClassLoader().getResource(GlobalConfiguration.FLINK_CONF_FILENAME); + Map<String, String> env = new HashMap<>(System.getenv()); + env.put( + ConfigConstants.ENV_FLINK_CONF_DIR, + new File(flinkConfigFileUrl.getFile()).getParentFile().getAbsolutePath()); + CommonTestUtils.setEnv(env); + FlinkAuronConfiguration config = new FlinkAuronConfiguration(); + assertEquals(config.get(AuronConfiguration.BATCH_SIZE), 9999); + assertEquals(config.get(AuronConfiguration.NATIVE_LOG_LEVEL), "DEBUG"); + assertEquals(config.get(AuronConfiguration.MEMORY_FRACTION), 0.6); // default value Review Comment: `CommonTestUtils.setEnv(env)` mutates the process-wide environment for the entire JVM. The test doesn’t restore the original environment afterwards, which can leak `FLINK_CONF_DIR` into other tests executed in the same fork. Capture the original env and restore it in a `finally` block or an `@AfterEach` cleanup. ```suggestion Map<String, String> originalEnv = new HashMap<>(System.getenv()); Map<String, String> env = new HashMap<>(originalEnv); env.put( ConfigConstants.ENV_FLINK_CONF_DIR, new File(flinkConfigFileUrl.getFile()).getParentFile().getAbsolutePath()); CommonTestUtils.setEnv(env); try { FlinkAuronConfiguration config = new FlinkAuronConfiguration(); assertEquals(config.get(AuronConfiguration.BATCH_SIZE), 9999); assertEquals(config.get(AuronConfiguration.NATIVE_LOG_LEVEL), "DEBUG"); assertEquals(config.get(AuronConfiguration.MEMORY_FRACTION), 0.6); // default value } finally { CommonTestUtils.setEnv(originalEnv); } ``` ########## auron-flink-extension/auron-flink-runtime/src/main/java/org/apache/auron/flink/configuration/FlinkAuronConfiguration.java: ########## @@ -0,0 +1,92 @@ +/* + * 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.auron.flink.configuration; + +import java.io.File; +import java.util.List; +import java.util.Optional; +import org.apache.auron.configuration.AuronConfiguration; +import org.apache.auron.configuration.ConfigOption; +import org.apache.flink.configuration.ConfigOptions; +import org.apache.flink.configuration.Configuration; +import org.apache.flink.configuration.GlobalConfiguration; + +/** + * Flink configuration proxy for Auron. + * All configuration prefixes start with flink. + */ +public class FlinkAuronConfiguration extends AuronConfiguration { + + // When using getOptional, the prefix will be automatically completed. If you only need to print the Option key, + // please manually add the prefix. + public static final String FLINK_PREFIX = "flink."; + private final Configuration flinkConfig; + + public FlinkAuronConfiguration() { + String pwd = System.getenv("PWD"); + if (new File(pwd + GlobalConfiguration.FLINK_CONF_FILENAME).exists()) { + // flink on yarn + flinkConfig = GlobalConfiguration.loadConfiguration(pwd); + } else { + // flink on k8s + flinkConfig = GlobalConfiguration.loadConfiguration(); Review Comment: `PWD` can be unset in many environments; when it is null, `loadConfiguration(pwd)` will throw. Also `new File(pwd + GlobalConfiguration.FLINK_CONF_FILENAME)` concatenates without a path separator (e.g., `/dirflink-conf.yaml`), so the existence check is very likely wrong and the YARN branch may never execute. Consider resolving the config directory via Flink’s standard `FLINK_CONF_DIR` env var (or `Paths.get(pwd, FLINK_CONF_FILENAME)` with null/separator handling) and only falling back when needed. ```suggestion // Prefer an explicit Flink configuration directory if provided, // then fall back to PWD (for YARN), and finally to Flink's default. String flinkConfDir = System.getenv("FLINK_CONF_DIR"); if (flinkConfDir != null && !flinkConfDir.isEmpty()) { // Explicit configuration directory flinkConfig = GlobalConfiguration.loadConfiguration(flinkConfDir); } else { String pwd = System.getenv("PWD"); if (pwd != null && !pwd.isEmpty() && new File(pwd, GlobalConfiguration.FLINK_CONF_FILENAME).exists()) { // flink on yarn: configuration file found in the working directory flinkConfig = GlobalConfiguration.loadConfiguration(pwd); } else { // flink on k8s or default environment flinkConfig = GlobalConfiguration.loadConfiguration(); } ``` ########## auron-flink-extension/auron-flink-runtime/src/test/java/org/apache/auron/flink/configuration/FlinkAuronConfigurationTest.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.auron.flink.configuration; + +import static org.junit.jupiter.api.Assertions.*; + +import java.io.File; +import java.net.URL; +import java.util.HashMap; +import java.util.Map; +import org.apache.auron.configuration.AuronConfiguration; +import org.apache.auron.flink.testutils.CommonTestUtils; +import org.apache.flink.configuration.ConfigConstants; +import org.apache.flink.configuration.GlobalConfiguration; +import org.junit.jupiter.api.Test; + +/** + * This class is used to test the FlinkAuronConfiguration class. + */ +public class FlinkAuronConfigurationTest { + + @Test + public void testGetConfigFromFlinkConfig() { + URL flinkConfigFileUrl = + FlinkAuronConfigurationTest.class.getClassLoader().getResource(GlobalConfiguration.FLINK_CONF_FILENAME); + Map<String, String> env = new HashMap<>(System.getenv()); + env.put( + ConfigConstants.ENV_FLINK_CONF_DIR, + new File(flinkConfigFileUrl.getFile()).getParentFile().getAbsolutePath()); + CommonTestUtils.setEnv(env); + FlinkAuronConfiguration config = new FlinkAuronConfiguration(); + assertEquals(config.get(AuronConfiguration.BATCH_SIZE), 9999); + assertEquals(config.get(AuronConfiguration.NATIVE_LOG_LEVEL), "DEBUG"); + assertEquals(config.get(AuronConfiguration.MEMORY_FRACTION), 0.6); // default value Review Comment: `assertEquals` arguments are reversed here (`assertEquals(expected, actual)`), which makes failure messages misleading. Swap the order so the constant is the expected value and the config lookup is the actual value. ```suggestion assertEquals(9999, config.get(AuronConfiguration.BATCH_SIZE)); assertEquals("DEBUG", config.get(AuronConfiguration.NATIVE_LOG_LEVEL)); assertEquals(0.6, config.get(AuronConfiguration.MEMORY_FRACTION)); // default value ``` -- 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]
