[incubator-kyuubi] branch master updated: [KYUUBI #1523] Refine usage about option '--namespace' in kyuubi-ctl

[ya...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

yao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-kyuubi.git


The following commit(s) were added to refs/heads/master by this push:
     new 8fc83e4  [KYUUBI #1523] Refine usage about option '--namespace' in kyuubi-ctl
8fc83e4 is described below

commit 8fc83e4e6936fbf57ff5fba9c30b5b5a832cd545
Author: sunfangbin <su...@cmss.chinamobile.com>
AuthorDate: Wed Dec 8 17:09:55 2021 +0800

    [KYUUBI #1523] Refine usage about option '--namespace' in kyuubi-ctl
    
    ### _Why are the changes needed?_
    The current option `-n` of command `kyuubi-ctl create` is confusing for new users. For example, when using the below command without setting `kyuubi.ha.zookeeper.namespace` it throws following exception which seems confused:
    ```
    $ bin/kyuubi-ctl create -s localhost -p 10009 -n test
    Only support expose Kyuubi server instance to another domain, but the default
    namespace is [None] and specified namespace is [test]
    
            at org.apache.kyuubi.ctl.ServiceControlCliArguments.fail(ServiceControlCliArguments.scala:294)
            at org.apache.kyuubi.ctl.ServiceControlCliArguments.validateCreateActionArguments(ServiceControlCliArguments.scala:195)
            at org.apache.kyuubi.ctl.ServiceControlCliArguments.validateArguments(ServiceControlCliArguments.scala:176)
            at org.apache.kyuubi.ctl.ServiceControlCliArguments.parse(ServiceControlCliArguments.scala:136)
            at org.apache.kyuubi.ctl.ServiceControlCliArguments.<init>(ServiceControlCliArguments.scala:36)
            ...
    ```
    In addtion, the description of option `-n` in `kyuubi-ctl --help` is `using kyuubi-defaults/conf if absent`, however when using below without option `-n` throws like:
    ```
    $ bin/kyuubi-ctl create -s localhost -p 10009
    Zookeeper namespace is not specified
    ...
    ```
    this because the default value is not copied to `cli.args` for `create` action. Users may confuse about this.
    
    ### _How was this patch tested?_
    - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
    
    - [ ] Add screenshots for manual tests if appropriate
    
    - [x] [Run test](https://kyuubi.readthedocs.io/en/latest/develop_tools/testing.html#running-tests) locally before make a pull request
    
    Closes #1523 from murong00/branch-1523.
    
    Closes #1523
    
    2439d467 [sunfangbin] Refine usage about option '--namespace' in kyuubi-ctl
    
    Authored-by: sunfangbin <su...@cmss.chinamobile.com>
    Signed-off-by: Kent Yao <ya...@apache.org>
---
 .../kyuubi/ctl/ServiceControlCliArguments.scala    | 17 ++++-------
 .../apache/kyuubi/ctl/ServiceControlCliSuite.scala | 34 ++++++++++++++++++++--
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala b/kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala
index 9f6ec00..dabefcf 100644
--- a/kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala
+++ b/kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala
@@ -151,9 +151,7 @@ class ServiceControlCliArguments(args: Seq[String], env: Map[String, String] = s
       }
     }
 
-    // for create action, it only expose Kyuubi service instance to another domain,
-    // so we do not use namespace from default conf
-    if (arguments.action != ServiceControlAction.CREATE && arguments.namespace == null) {
+    if (arguments.namespace == null) {
       arguments = arguments.copy(namespace = conf.get(HA_ZK_NAMESPACE))
       if (arguments.verbose) {
         super.info(s"Zookeeper namespace is not specified, use value from default conf:" +
@@ -188,11 +186,12 @@ class ServiceControlCliArguments(args: Seq[String], env: Map[String, String] = s
     validateZkArguments()
 
     val defaultNamespace = conf.getOption(HA_ZK_NAMESPACE.key)
-    if (defaultNamespace.isEmpty || defaultNamespace.get.equals(cliArgs.namespace)) {
+      .getOrElse(HA_ZK_NAMESPACE.defaultValStr)
+    if (defaultNamespace.equals(cliArgs.namespace)) {
       fail(
         s"""
-           |Only support expose Kyuubi server instance to another domain, but the default
-           |namespace is [$defaultNamespace] and specified namespace is [${cliArgs.namespace}]
+           |Only support expose Kyuubi server instance to another domain, a different namespace
+           |than the default namespace [$defaultNamespace] should be specified.
         """.stripMargin)
     }
   }
@@ -223,11 +222,7 @@ class ServiceControlCliArguments(args: Seq[String], env: Map[String, String] = s
       fail("Zookeeper quorum is not specified and no default value to load")
     }
     if (cliArgs.namespace == null) {
-      if (cliArgs.action == ServiceControlAction.CREATE) {
-        fail("Zookeeper namespace is not specified")
-      } else {
-        fail("Zookeeper namespace is not specified and no default value to load")
-      }
+      fail("Zookeeper namespace is not specified and no default value to load")
     }
   }
 
diff --git a/kyuubi-ctl/src/test/scala/org/apache/kyuubi/ctl/ServiceControlCliSuite.scala b/kyuubi-ctl/src/test/scala/org/apache/kyuubi/ctl/ServiceControlCliSuite.scala
index 7703e48..8226426 100644
--- a/kyuubi-ctl/src/test/scala/org/apache/kyuubi/ctl/ServiceControlCliSuite.scala
+++ b/kyuubi-ctl/src/test/scala/org/apache/kyuubi/ctl/ServiceControlCliSuite.scala
@@ -133,7 +133,7 @@ class ServiceControlCliSuite extends KyuubiFunSuite with TestPrematureExit {
     }
   }
 
-  test("test expose to same namespace or not specified namespace") {
+  test("test expose to same namespace") {
     conf
       .unset(KyuubiConf.SERVER_KEYTAB)
       .unset(KyuubiConf.SERVER_PRINCIPAL)
@@ -153,6 +153,14 @@ class ServiceControlCliSuite extends KyuubiFunSuite with TestPrematureExit {
       "--port",
       port)
     testPrematureExit(args, "Only support expose Kyuubi server instance to another domain")
+  }
+
+  test("test not specified namespace") {
+    conf
+      .unset(KyuubiConf.SERVER_KEYTAB)
+      .unset(KyuubiConf.SERVER_PRINCIPAL)
+      .set(HA_ZK_QUORUM, zkServer.getConnectString)
+      .set(KyuubiConf.FRONTEND_THRIFT_BINARY_BIND_PORT, 0)
 
     val args2 = Array(
       "create",
@@ -163,7 +171,29 @@ class ServiceControlCliSuite extends KyuubiFunSuite with TestPrematureExit {
       host,
       "--port",
       port)
-    testPrematureExit(args2, "Zookeeper namespace is not specified")
+    testPrematureExit(args2, "Only support expose Kyuubi server instance to another domain")
+  }
+
+  test("test expose to another namespace") {
+    conf
+      .unset(KyuubiConf.SERVER_KEYTAB)
+      .unset(KyuubiConf.SERVER_PRINCIPAL)
+      .set(HA_ZK_QUORUM, zkServer.getConnectString)
+      .set(HA_ZK_NAMESPACE, namespace)
+      .set(KyuubiConf.FRONTEND_THRIFT_BINARY_BIND_PORT, 0)
+
+    val args = Array(
+      "create",
+      "server",
+      "--zk-quorum",
+      zkServer.getConnectString,
+      "--namespace",
+      "other-kyuubi-server",
+      "--host",
+      host,
+      "--port",
+      port)
+    testPrematureExit(args, "")
   }
 
   test("test render zookeeper service node info") {