From d6ee3c18bc4855980f52e8ef43fef99752bf6fec Mon Sep 17 00:00:00 2001 From: Binjie Yang <52876270+zwangsheng@users.noreply.github.com> Date: Fri, 2 Dec 2022 22:53:28 +0800 Subject: [PATCH] [CELEBORN-98][IMPROVEMENT] Remove unreachable code block in master/work arguments (#1042) --- .../deploy/master/MasterArguments.scala | 4 -- .../deploy/master/MasterArgumentsSuite.scala | 53 +++++++++++++++++++ .../deploy/worker/WorkerArguments.scala | 4 -- .../service/deploy/WorkerArgumentsSuite.scala | 52 ++++++++++++++++++ 4 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 master/src/test/scala/org/apache/celeborn/service/deploy/master/MasterArgumentsSuite.scala create mode 100644 worker/src/test/scala/org/apache/celeborn/service/deploy/WorkerArgumentsSuite.scala diff --git a/master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala b/master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala index 86baae8ce..7b0d07e70 100644 --- a/master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala +++ b/master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala @@ -51,10 +51,6 @@ class MasterArguments(args: Array[String], conf: CelebornConf) { _port = _port.orElse(Some(conf.masterPort)) } - if (_host.isEmpty || _port.isEmpty) { - printUsageAndExit(1) - } - def host: String = _host.get def port: Int = _port.get diff --git a/master/src/test/scala/org/apache/celeborn/service/deploy/master/MasterArgumentsSuite.scala b/master/src/test/scala/org/apache/celeborn/service/deploy/master/MasterArgumentsSuite.scala new file mode 100644 index 000000000..c32d6fe32 --- /dev/null +++ b/master/src/test/scala/org/apache/celeborn/service/deploy/master/MasterArgumentsSuite.scala @@ -0,0 +1,53 @@ +/* + * 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.celeborn.service.deploy.master + +import org.scalatest.funsuite.AnyFunSuite + +import org.apache.celeborn.common.CelebornConf +import org.apache.celeborn.common.CelebornConf.{MASTER_HOST, MASTER_PORT} +import org.apache.celeborn.common.internal.Logging +import org.apache.celeborn.common.util.Utils + +class MasterArgumentsSuite extends AnyFunSuite with Logging { + + test("[CELEBORN-98] Test build masterArguments in different case") { + val args1 = Array.empty[String] + val conf1 = new CelebornConf() + + val arguments1 = new MasterArguments(args1, conf1) + assert(arguments1.host.equals(Utils.localHostName)) + assert(arguments1.port == 9097) + + // should use celeborn conf + val conf2 = new CelebornConf() + conf2.set(MASTER_HOST, "test-host-1") + conf2.set(MASTER_PORT, 19097) + + val arguments2 = new MasterArguments(args1, conf2) + assert(arguments2.host.equals("test-host-1")) + assert(arguments2.port == 19097) + + // should use cli args + val args2 = Array("-h", "test-host-2", "-p", "29097") + + val arguments3 = new MasterArguments(args2, conf2) + assert(arguments3.host.equals("test-host-2")) + assert(arguments3.port == 29097) + } +} diff --git a/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/WorkerArguments.scala b/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/WorkerArguments.scala index f6b27c3e7..76280132a 100644 --- a/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/WorkerArguments.scala +++ b/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/WorkerArguments.scala @@ -38,10 +38,6 @@ class WorkerArguments(args: Array[String], conf: CelebornConf) { _host = _host.orElse(Some(Utils.localHostName)) _port = _port.orElse(Some(conf.workerRpcPort)) - if (_host.isEmpty || _port.isEmpty) { - printUsageAndExit(1) - } - def host: String = _host.get def port: Int = _port.get diff --git a/worker/src/test/scala/org/apache/celeborn/service/deploy/WorkerArgumentsSuite.scala b/worker/src/test/scala/org/apache/celeborn/service/deploy/WorkerArgumentsSuite.scala new file mode 100644 index 000000000..a092c1b11 --- /dev/null +++ b/worker/src/test/scala/org/apache/celeborn/service/deploy/WorkerArgumentsSuite.scala @@ -0,0 +1,52 @@ +/* + * 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.celeborn.service.deploy + +import org.scalatest.funsuite.AnyFunSuite + +import org.apache.celeborn.common.CelebornConf +import org.apache.celeborn.common.CelebornConf.WORKER_RPC_PORT +import org.apache.celeborn.common.internal.Logging +import org.apache.celeborn.common.util.Utils +import org.apache.celeborn.service.deploy.worker.WorkerArguments + +class WorkerArgumentsSuite extends AnyFunSuite with Logging { + + test("[CELEBORN-98] Test build workerArguments in different case") { + val args1 = Array.empty[String] + val conf1 = new CelebornConf() + + val arguments1 = new WorkerArguments(args1, conf1) + assert(arguments1.host.equals(Utils.localHostName)) + assert(arguments1.port == 0) + + // should use celeborn conf + val conf2 = new CelebornConf() + conf2.set(WORKER_RPC_PORT, 12345) + + val arguments2 = new WorkerArguments(args1, conf2) + assert(arguments2.port == 12345) + + // should use cli args + val args2 = Array("-h", "test-host-1", "-p", "22345") + + val arguments3 = new WorkerArguments(args2, conf2) + assert(arguments3.host.equals("test-host-1")) + assert(arguments3.port == 22345) + } +}