Copilot commented on code in PR #6528: URL: https://github.com/apache/texera/pull/6528#discussion_r3609356948
########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/WinutilsFreeLocalFileSystemSpec.scala: ########## @@ -0,0 +1,84 @@ +/* + * 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.texera.amber.util + +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.permission.FsPermission +import org.apache.hadoop.fs.{FileSystem, Path} +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import java.nio.charset.StandardCharsets +import java.nio.file.Files + +class WinutilsFreeLocalFileSystemSpec extends AnyFlatSpec { + + private def newFs(): FileSystem = { + val conf = new Configuration() + // Select the file system the same way production code does: through `fs.file.impl`. + conf.set("fs.file.impl", classOf[WinutilsFreeLocalFileSystem].getName) + conf.setBoolean("fs.file.impl.disable.cache", true) + FileSystem.get(URI.create("file:///"), conf) + } + + "WinutilsFreeLocalFileSystem" should "be selected through fs.file.impl" in { + assert(newFs().isInstanceOf[WinutilsFreeLocalFileSystem]) + } Review Comment: `newFs()` returns a non-cached Hadoop `FileSystem` instance; the test currently never closes it, which can leak resources and cause flakiness on Windows due to file handles. Close the FileSystem in a `finally` block. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/WinutilsFreeLocalFileSystemSpec.scala: ########## @@ -0,0 +1,84 @@ +/* + * 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.texera.amber.util + +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.permission.FsPermission +import org.apache.hadoop.fs.{FileSystem, Path} +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import java.nio.charset.StandardCharsets +import java.nio.file.Files + +class WinutilsFreeLocalFileSystemSpec extends AnyFlatSpec { + + private def newFs(): FileSystem = { + val conf = new Configuration() + // Select the file system the same way production code does: through `fs.file.impl`. + conf.set("fs.file.impl", classOf[WinutilsFreeLocalFileSystem].getName) + conf.setBoolean("fs.file.impl.disable.cache", true) + FileSystem.get(URI.create("file:///"), conf) + } + + "WinutilsFreeLocalFileSystem" should "be selected through fs.file.impl" in { + assert(newFs().isInstanceOf[WinutilsFreeLocalFileSystem]) + } + + it should "create directories and read/write files without winutils" in { + val fs = newFs() + val tmp = Files.createTempDirectory("winutils-free-fs") + val dir = new Path(tmp.toUri.toString, "a/b/c") + assert(fs.mkdirs(dir)) + + val file = new Path(dir, "data.txt") + val out = fs.create(file, true) + out.write("hello".getBytes(StandardCharsets.UTF_8)) + out.close() + + val in = fs.open(file) + val buf = new Array[Byte](5) + in.readFully(buf) + in.close() + assert(new String(buf, StandardCharsets.UTF_8) == "hello") + + assert(fs.getFileStatus(file).getLen == 5) + assert(fs.delete(file, false)) + } + + it should "not fail on setPermission" in { + val fs = newFs() + val tmp = Files.createTempDirectory("winutils-free-fs-perm") + val file = new Path(tmp.toUri.toString, "perm.txt") + fs.create(file, true).close() + // No-op on Windows hosts without winutils; delegates to Hadoop's default elsewhere. + fs.setPermission(file, new FsPermission("755")) + } Review Comment: The `FileSystem` instance created here is never closed. Closing it helps prevent test flakiness/resource leaks, particularly on Windows where open handles can block cleanup. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/WinutilsFreeLocalFileSystemSpec.scala: ########## @@ -0,0 +1,84 @@ +/* + * 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.texera.amber.util + +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.permission.FsPermission +import org.apache.hadoop.fs.{FileSystem, Path} +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import java.nio.charset.StandardCharsets +import java.nio.file.Files + +class WinutilsFreeLocalFileSystemSpec extends AnyFlatSpec { + + private def newFs(): FileSystem = { + val conf = new Configuration() + // Select the file system the same way production code does: through `fs.file.impl`. + conf.set("fs.file.impl", classOf[WinutilsFreeLocalFileSystem].getName) + conf.setBoolean("fs.file.impl.disable.cache", true) + FileSystem.get(URI.create("file:///"), conf) + } + + "WinutilsFreeLocalFileSystem" should "be selected through fs.file.impl" in { + assert(newFs().isInstanceOf[WinutilsFreeLocalFileSystem]) + } + + it should "create directories and read/write files without winutils" in { + val fs = newFs() + val tmp = Files.createTempDirectory("winutils-free-fs") + val dir = new Path(tmp.toUri.toString, "a/b/c") + assert(fs.mkdirs(dir)) + + val file = new Path(dir, "data.txt") + val out = fs.create(file, true) + out.write("hello".getBytes(StandardCharsets.UTF_8)) + out.close() + + val in = fs.open(file) + val buf = new Array[Byte](5) + in.readFully(buf) + in.close() + assert(new String(buf, StandardCharsets.UTF_8) == "hello") + + assert(fs.getFileStatus(file).getLen == 5) + assert(fs.delete(file, false)) + } Review Comment: This test creates a Hadoop `FileSystem` but never closes it. Since caching is disabled (`fs.file.impl.disable.cache=true`), each call creates a new instance that should be closed to avoid leaking file handles/resources (especially on Windows). ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/IcebergUtilSpec.scala: ########## @@ -300,6 +303,43 @@ class IcebergUtilSpec extends AnyFlatSpec { assert(IcebergUtil.fromRecord(record, schema) == tuple) } + it should "select WinutilsFreeLocalFileSystem when winutils is unavailable on Windows" in { + val conf = IcebergUtil.newLocalHadoopConf(useWinutilsFreeLocalFs = true) + assert(conf.get("fs.file.impl") == classOf[WinutilsFreeLocalFileSystem].getName) + assert(conf.getBoolean("fs.file.impl.disable.cache", false)) + assert( + FileSystem.get(URI.create("file:///"), conf).isInstanceOf[WinutilsFreeLocalFileSystem] + ) + } Review Comment: This test instantiates a Hadoop `FileSystem` via `FileSystem.get(...)` but never closes it. Since the config disables the cache, it creates a fresh FS instance that should be closed to avoid resource leaks. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/WinutilsFreeLocalFileSystemSpec.scala: ########## @@ -0,0 +1,84 @@ +/* + * 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.texera.amber.util + +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.permission.FsPermission +import org.apache.hadoop.fs.{FileSystem, Path} +import org.scalatest.flatspec.AnyFlatSpec + +import java.net.URI +import java.nio.charset.StandardCharsets +import java.nio.file.Files + +class WinutilsFreeLocalFileSystemSpec extends AnyFlatSpec { + + private def newFs(): FileSystem = { + val conf = new Configuration() + // Select the file system the same way production code does: through `fs.file.impl`. + conf.set("fs.file.impl", classOf[WinutilsFreeLocalFileSystem].getName) + conf.setBoolean("fs.file.impl.disable.cache", true) + FileSystem.get(URI.create("file:///"), conf) + } + + "WinutilsFreeLocalFileSystem" should "be selected through fs.file.impl" in { + assert(newFs().isInstanceOf[WinutilsFreeLocalFileSystem]) + } + + it should "create directories and read/write files without winutils" in { + val fs = newFs() + val tmp = Files.createTempDirectory("winutils-free-fs") + val dir = new Path(tmp.toUri.toString, "a/b/c") + assert(fs.mkdirs(dir)) + + val file = new Path(dir, "data.txt") + val out = fs.create(file, true) + out.write("hello".getBytes(StandardCharsets.UTF_8)) + out.close() + + val in = fs.open(file) + val buf = new Array[Byte](5) + in.readFully(buf) + in.close() + assert(new String(buf, StandardCharsets.UTF_8) == "hello") + + assert(fs.getFileStatus(file).getLen == 5) + assert(fs.delete(file, false)) + } + + it should "not fail on setPermission" in { + val fs = newFs() + val tmp = Files.createTempDirectory("winutils-free-fs-perm") + val file = new Path(tmp.toUri.toString, "perm.txt") + fs.create(file, true).close() + // No-op on Windows hosts without winutils; delegates to Hadoop's default elsewhere. + fs.setPermission(file, new FsPermission("755")) + } + + it should "not fail on setOwner" in { + val fs = newFs() + val tmp = Files.createTempDirectory("winutils-free-fs-owner") + val file = new Path(tmp.toUri.toString, "owner.txt") + fs.create(file, true).close() + // Chown-to-self is permitted everywhere; no-op on Windows hosts without winutils. + // (Not FileStatus.getOwner: reading the owner itself requires winutils on Windows.) + fs.setOwner(file, System.getProperty("user.name"), null) + } Review Comment: The `FileSystem` instance created here is never closed. Please close it in a `finally` block to avoid leaking resources across tests. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/IcebergUtilSpec.scala: ########## @@ -300,6 +303,43 @@ class IcebergUtilSpec extends AnyFlatSpec { assert(IcebergUtil.fromRecord(record, schema) == tuple) } + it should "select WinutilsFreeLocalFileSystem when winutils is unavailable on Windows" in { + val conf = IcebergUtil.newLocalHadoopConf(useWinutilsFreeLocalFs = true) + assert(conf.get("fs.file.impl") == classOf[WinutilsFreeLocalFileSystem].getName) + assert(conf.getBoolean("fs.file.impl.disable.cache", false)) + assert( + FileSystem.get(URI.create("file:///"), conf).isInstanceOf[WinutilsFreeLocalFileSystem] + ) + } + + it should "leave the Hadoop configuration untouched when winutils is not needed" in { + val conf = IcebergUtil.newLocalHadoopConf(useWinutilsFreeLocalFs = false) + assert(conf.get("fs.file.impl") == null) + assert(!conf.getBoolean("fs.file.impl.disable.cache", false)) + } + + it should "create and load tables via createHadoopCatalog in a local warehouse on every platform" in { + val warehouse = Files.createTempDirectory("iceberg-local-warehouse") + val catalog = IcebergUtil.createHadoopCatalog("local_test", warehouse) + + // On Windows hosts without a winutils.exe installation, this used to fail with + // "Hadoop bin directory does not exist": Hadoop's default local file system + // shells out to winutils for chmod on every file/directory creation. + IcebergUtil.createTable( + catalog, + "test_namespace", + "test_table", + IcebergUtil.toIcebergSchema(Schema().add("id", AttributeType.INTEGER)), + overrideIfExists = true + ) + + assert( + Files.exists(warehouse.resolve("test_namespace").resolve("test_table").resolve("metadata")), + "table metadata must be written through the local file system" + ) + assert(IcebergUtil.loadTableMetadata(catalog, "test_namespace", "test_table").nonEmpty) + } Review Comment: This test creates a `HadoopCatalog` but never closes it. Other Iceberg-related specs close catalogs in `afterAll`; doing so here avoids leaking underlying Hadoop `FileSystem` instances and file handles across the test suite. -- 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]
