aglinxinyuan commented on code in PR #6528: URL: https://github.com/apache/texera/pull/6528#discussion_r3612294603
########## 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: Good catch — the four tests now go through a withFs loan helper that closes the (uncached) FileSystem in a finally, so no handle leaks across the suite. ########## 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: Good catch — the four tests now go through a withFs loan helper that closes the (uncached) FileSystem in a finally, so no handle leaks across the suite. ########## 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: Good catch — the four tests now go through a withFs loan helper that closes the (uncached) FileSystem in a finally, so no handle leaks across the suite. ########## 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: Good catch — the four tests now go through a withFs loan helper that closes the (uncached) FileSystem in a finally, so no handle leaks across the suite. ########## 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: Fixed — the FileSystem is now assigned to a val and closed in a finally block. ########## 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: Fixed — the HadoopCatalog is now closed in a finally (via the same case closeable: AutoCloseable pattern IcebergTableWriterSpec uses), releasing its underlying FileSystem. -- 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]
