aglinxinyuan commented on code in PR #6094: URL: https://github.com/apache/texera/pull/6094#discussion_r3522748794
########## common/config/src/test/scala/org/apache/texera/common/config/PythonUtilsSpec.scala: ########## @@ -0,0 +1,49 @@ +/* + * 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.common.config + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[PythonUtils]]. `getPythonExecutable` falls back to "python3" when the configured + * python path is blank, else returns the trimmed path. The blank-path default depends on + * UDF_PYTHON_PATH, so the exact-value assertion is guarded on that override being unset. + */ +class PythonUtilsSpec extends AnyFlatSpec with Matchers { + + "PythonUtils.getPythonExecutable" should "fall back to python3 when no python path is configured" in { + if (sys.env.get("UDF_PYTHON_PATH").forall(_.trim.isEmpty)) { + PythonUtils.getPythonExecutable shouldBe "python3" + } + } Review Comment: Done — the guard now treats `${?UDF_PYTHON_PATH}` as set if present in either `sys.env` or `sys.props`. ########## common/config/src/test/scala/org/apache/texera/common/config/PekkoConfigSpec.scala: ########## @@ -0,0 +1,75 @@ +/* + * 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.common.config + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[PekkoConfig]]. Reading keys off the returned config forces resolution of cluster.conf + * (merged with the Typesafe default application config), so a renamed or mistyped key surfaces here + * as a ConfigException. The log level carries a `${?ENV}` override, so its assertion is guarded. + */ +class PekkoConfigSpec extends AnyFlatSpec with Matchers { + + "PekkoConfig.pekkoConfig" should "load the actor/serialization settings from cluster.conf" in { + val config = PekkoConfig.pekkoConfig + config.getString("pekko.actor.provider") shouldBe "cluster" + config.getBoolean("pekko.actor.allow-java-serialization") shouldBe false + config.getBoolean("pekko.actor.enable-additional-serialization-bindings") shouldBe true + config.getString( + "pekko.actor.serializers.kryo" + ) shouldBe "io.altoo.serialization.kryo.pekko.PekkoKryoSerializer" + config.getStringList("pekko.loggers").get(0) shouldBe "org.apache.pekko.event.slf4j.Slf4jLogger" + config.getString( + "pekko.logging-filter" + ) shouldBe "org.apache.pekko.event.slf4j.Slf4jLoggingFilter" + } + + it should "expose the remote/artery transport settings" in { + val config = PekkoConfig.pekkoConfig + config.getString("pekko.remote.artery.transport") shouldBe "tcp" + config.getString("pekko.remote.artery.canonical.hostname") shouldBe "0.0.0.0" + config.getInt("pekko.remote.artery.canonical.port") shouldBe 0 + config.getBytes("pekko.remote.artery.advanced.maximum-frame-size") shouldBe 31457280L + config.getBytes("pekko.remote.artery.advanced.maximum-large-frame-size") shouldBe 125829120L + } + + it should "expose the cluster and failure-detector settings" in { + val config = PekkoConfig.pekkoConfig + config.getStringList("pekko.cluster.seed-nodes").size shouldBe 0 + config.getString( + "pekko.cluster.downing-provider-class" + ) shouldBe "org.apache.pekko.cluster.sbr.SplitBrainResolverProvider" + config.getString("pekko.cluster.gossip-interval") shouldBe "10s" + config.getString("pekko.cluster.failure-detector.acceptable-heartbeat-pause") shouldBe "50s" + config.getString( + "pekko-kryo-serialization.kryo-initializer" + ) shouldBe "org.apache.texera.amber.engine.common.AmberKryoInitializer" + } + + it should "resolve the log levels" in { + val config = PekkoConfig.pekkoConfig + if (sys.env.get("TEXERA_SERVICE_LOG_LEVEL").isEmpty) { + config.getString("pekko.loglevel") shouldBe "INFO" + } + config.getString("pekko.stdout-loglevel") shouldBe "INFO" + } Review Comment: Done — the loglevel guard now checks both `sys.env` and `sys.props` for TEXERA_SERVICE_LOG_LEVEL. ########## common/config/src/test/scala/org/apache/texera/common/config/UdfConfigSpec.scala: ########## @@ -0,0 +1,49 @@ +/* + * 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.common.config + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Spec for [[UdfConfig]]. Reading each value forces resolution from udf.conf, so a renamed or + * mistyped key surfaces here as a ConfigException. Every value carries a `${?ENV}` override, so + * exact-value assertions are guarded on the override being absent from env and system properties. + */ +class UdfConfigSpec extends AnyFlatSpec with Matchers { + + private def ifUnset(name: String)(assertion: => Any): Unit = + if (!sys.env.contains(name) && !sys.props.contains(name)) assertion + + "UdfConfig" should "resolve the python and R defaults from udf.conf" in { + ifUnset("UDF_PYTHON_PATH")(UdfConfig.pythonPath shouldBe "") + ifUnset("UDF_R_PATH")(UdfConfig.rPath shouldBe "") + ifUnset("UDF_PYTHON_LOG_STREAMHANDLER_LEVEL")( + UdfConfig.pythonLogStreamHandlerLevel shouldBe "INFO" + ) + ifUnset("UDF_PYTHON_LOG_FILEHANDLER_DIR")(UdfConfig.pythonLogFileHandlerDir shouldBe "/tmp/") + ifUnset("UDF_PYTHON_LOG_FILEHANDLER_LEVEL")(UdfConfig.pythonLogFileHandlerLevel shouldBe "INFO") + } + + it should "resolve the non-empty log format strings" in { + UdfConfig.pythonLogStreamHandlerFormat should not be empty + UdfConfig.pythonLogFileHandlerFormat should not be empty + } Review Comment: Done — the log-format assertions are now guarded on UDF_PYTHON_LOG_*_FORMAT via the env+system-property-aware `ifUnset` helper. -- 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]
