LuciferYang commented on code in PR #13098:
URL: https://github.com/apache/gluten/pull/13098#discussion_r4082669208
##########
gluten-core/src/main/scala/org/apache/spark/util/SparkDirectoryUtil.scala:
##########
@@ -57,8 +56,14 @@ class SparkDirectoryUtil private (val roots: Array[String])
extends Logging {
private val NAMESPACE_MAPPING: java.util.Map[String, Namespace] =
new ConcurrentHashMap[String, Namespace]
- def namespace(name: String): Namespace =
+ def namespace(name: String): Namespace = {
+ if (ROOTS.isEmpty) {
+ throw new IllegalStateException(
+ s"No available Gluten local directory for namespace '$name': none of
the configured " +
+ s"local directories ${roots.mkString(", ")} could be created")
Review Comment:
Reworded to list each configured directory quoted and bracketed (`['a',
'b']`), and to state explicitly when none were configured. Fixed in 1156c296e.
##########
gluten-core/src/main/scala/org/apache/spark/util/SparkDirectoryUtil.scala:
##########
@@ -98,16 +103,19 @@ class Namespace(private val parents: Array[File], private
val name: String) {
path.toFile
}
+ if (all.isEmpty) {
+ throw new IllegalStateException(
+ s"No available Gluten local directory in namespace '$name'")
Review Comment:
This safety net only fires when the parent list is empty (`all.isEmpty` iff
`parents.isEmpty`, since `all` is a length-preserving map of `parents`), so
there is no non-empty list to print. Reworded it to state that no parent
directories were provided, which is the actionable detail. Fixed in 1156c296e.
##########
gluten-core/src/test/scala/org/apache/spark/util/SparkDirectoryUtilSuite.scala:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.spark.util
+
+import org.apache.spark.SparkConf
+
+import org.scalatest.funsuite.AnyFunSuite
+
+import java.io.File
+import java.nio.file.Files
+
+class SparkDirectoryUtilSuite extends AnyFunSuite {
+
+ test("namespace fails fast naming the configured local dirs when none can be
created") {
+ // A regular file cannot host a local directory, so directory creation
fails
+ // for every configured root.
+ val notADir = Files.createTempFile("gluten-not-a-dir", ".tmp")
+ notADir.toFile.deleteOnExit()
+ val conf = new SparkConf()
+ .set("spark.master", "local[1]")
+ .set("spark.local.dirs", notADir.toAbsolutePath.toString)
+ .set("spark.local.dir", notADir.toAbsolutePath.toString)
+ SparkDirectoryUtil.init(conf)
+ val exception = intercept[IllegalStateException] {
+ SparkDirectoryUtil.get().namespace("test-namespace")
+ }
Review Comment:
Keeping the test on the real `init` + `namespace` path. The suite passes in
CI, and adding a `resetForTesting`/`createForTesting` hook to production solely
for the test would widen the public API for a theoretical ordering issue. I
would rather not grow production surface for that; if the suite ever proves
order-dependent in practice we can revisit.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
