Merge pull request #54708 from ClickHouse/keeper-client-safe-delete

Keeper client safe delete

docs/en/operations/utilities/clickhouse-keeper-client.md

@@ -45,13 +45,13 @@ keeper foo bar
 ## Commands {#clickhouse-keeper-client-commands}
 
 -   `ls [path]` -- Lists the nodes for the given path (default: cwd)
--   `cd [path]` -- Change the working path (default `.`)
+-   `cd [path]` -- Changes the working path (default `.`)
 -   `exists <path>` -- Returns `1` if node exists, `0` otherwise
--   `set <path> <value> [version]` -- Updates the node's value. Only update if version matches (default: -1)
+-   `set <path> <value> [version]` -- Updates the node's value. Only updates if version matches (default: -1)
 -   `create <path> <value> [mode]` -- Creates new node with the set value
 -   `touch <path>` -- Creates new node with an empty string as value. Doesn't throw an exception if the node already exists
 -   `get <path>` -- Returns the node's value
--   `remove <path>` -- Remove the node
+-   `rm <path> [version]` -- Removes the node only if version matches (default: -1)
 -   `rmr <path>` -- Recursively deletes path. Confirmation required
 -   `flwc <command>` -- Executes four-letter-word command
 -   `help` -- Prints this message

programs/keeper-client/Commands.cpp

@@ -347,12 +347,20 @@ bool RMCommand::parse(IParser::Pos & pos, std::shared_ptr<ASTKeeperQuery> & node
         return false;
     node->args.push_back(std::move(path));
 
+    ASTPtr version;
+    if (ParserNumber{}.parse(pos, version, expected))
+        node->args.push_back(version->as<ASTLiteral &>().value);
+
     return true;
 }
 
 void RMCommand::execute(const ASTKeeperQuery * query, KeeperClient * client) const
 {
-    client->zookeeper->remove(client->getAbsolutePath(query->args[0].safeGet<String>()));
+    Int32 version{-1};
+    if (query->args.size() == 2)
+        version = static_cast<Int32>(query->args[1].get<Int32>());
+
+    client->zookeeper->remove(client->getAbsolutePath(query->args[0].safeGet<String>()), version);
 }
 
 bool RMRCommand::parse(IParser::Pos & pos, std::shared_ptr<ASTKeeperQuery> & node, Expected & expected) const
@@ -368,8 +376,8 @@ bool RMRCommand::parse(IParser::Pos & pos, std::shared_ptr<ASTKeeperQuery> & nod
 void RMRCommand::execute(const ASTKeeperQuery * query, KeeperClient * client) const
 {
     String path = client->getAbsolutePath(query->args[0].safeGet<String>());
-    client->askConfirmation("You are going to recursively delete path " + path,
-                            [client, path]{ client->zookeeper->removeRecursive(path); });
+    client->askConfirmation(
+        "You are going to recursively delete path " + path, [client, path] { client->zookeeper->removeRecursive(path); });
 }
 
 bool ReconfigCommand::parse(IParser::Pos & pos, std::shared_ptr<ASTKeeperQuery> & node, DB::Expected & expected) const

programs/keeper-client/Commands.h

@@ -51,7 +51,7 @@ class CDCommand : public IKeeperClientCommand
 
     void execute(const ASTKeeperQuery * query, KeeperClient * client) const override;
 
-    String getHelpMessage() const override { return "{} [path] -- Change the working path (default `.`)"; }
+    String getHelpMessage() const override { return "{} [path] -- Changes the working path (default `.`)"; }
 };
 
 class SetCommand : public IKeeperClientCommand
@@ -64,7 +64,7 @@ class SetCommand : public IKeeperClientCommand
 
     String getHelpMessage() const override
     {
-        return "{} <path> <value> [version] -- Updates the node's value. Only update if version matches (default: -1)";
+        return "{} <path> <value> [version] -- Updates the node's value. Only updates if version matches (default: -1)";
     }
 };
 
@@ -165,7 +165,6 @@ class FindBigFamily : public IKeeperClientCommand
     }
 };
 
-
 class RMCommand : public IKeeperClientCommand
 {
     String getName() const override { return "rm"; }
@@ -174,7 +173,7 @@ class RMCommand : public IKeeperClientCommand
 
     void execute(const ASTKeeperQuery * query, KeeperClient * client) const override;
 
-    String getHelpMessage() const override { return "{} <path> -- Remove the node"; }
+    String getHelpMessage() const override { return "{} <path> [version] -- Removes the node only if version matches (default: -1)"; }
 };
 
 class RMRCommand : public IKeeperClientCommand

programs/keeper-client/Parser.cpp

@@ -11,8 +11,6 @@ bool parseKeeperArg(IParser::Pos & pos, Expected & expected, String & result)
     {
         if (!parseIdentifierOrStringLiteral(pos, expected, result))
             return false;
-
-        ParserToken{TokenType::Whitespace}.ignore(pos);
     }
 
     while (pos->type != TokenType::Whitespace && pos->type != TokenType::EndOfStream && pos->type != TokenType::Semicolon)

tests/integration/helpers/keeper_utils.py

@@ -115,6 +115,14 @@ class KeeperClient(object):
     def get(self, path: str, timeout: float = 60.0) -> str:
         return self.execute_query(f"get {path}", timeout)
 
+    def set(self, path: str, value: str, version: tp.Optional[int] = None) -> None:
+        self.execute_query(
+            f"set {path} {value} {version if version is not None else ''}"
+        )
+
+    def rm(self, path: str, version: tp.Optional[int] = None) -> None:
+        self.execute_query(f"rm {path} {version if version is not None else ''}")
+
     def exists(self, path: str, timeout: float = 60.0) -> bool:
         return bool(int(self.execute_query(f"exists {path}", timeout)))
 

tests/integration/test_keeper_client/test.py

@@ -1,7 +1,7 @@
 import pytest
 from helpers.cluster import ClickHouseCluster
 from helpers.test_tools import TSV
-from helpers.keeper_utils import KeeperClient
+from helpers.keeper_utils import KeeperClient, KeeperException
 
 
 cluster = ClickHouseCluster(__file__)
@@ -129,3 +129,90 @@ def test_base_commands(client: KeeperClient):
 
 def test_four_letter_word_commands(client: KeeperClient):
     assert client.execute_query("ruok") == "imok"
+
+
+def test_rm_with_version(client: KeeperClient):
+    node_path = "/test_rm_with_version_node"
+    client.create(node_path, "value")
+    assert client.get(node_path) == "value"
+
+    with pytest.raises(KeeperException) as ex:
+        client.rm(node_path, 1)
+
+    ex_as_str = str(ex)
+    assert "Coordination error: Bad version" in ex_as_str
+    assert node_path in ex_as_str
+    assert client.get(node_path) == "value"
+
+    client.rm(node_path, 0)
+
+    with pytest.raises(KeeperException) as ex:
+        client.get(node_path)
+
+    ex_as_str = str(ex)
+    assert "node doesn't exist" in ex_as_str
+    assert node_path in ex_as_str
+
+
+def test_rm_without_version(client: KeeperClient):
+    node_path = "/test_rm_with_version_node"
+    client.create(node_path, "value")
+    assert client.get(node_path) == "value"
+
+    client.rm(node_path)
+
+    with pytest.raises(KeeperException) as ex:
+        client.get(node_path)
+
+    ex_as_str = str(ex)
+    assert "node doesn't exist" in ex_as_str
+    assert node_path in ex_as_str
+
+
+def test_set_with_version(client: KeeperClient):
+    node_path = "/test_set_with_version_node"
+    client.create(node_path, "value")
+    assert client.get(node_path) == "value"
+
+    client.set(node_path, "value1", 0)
+    assert client.get(node_path) == "value1"
+
+    with pytest.raises(KeeperException) as ex:
+        client.set(node_path, "value2", 2)
+
+    ex_as_str = str(ex)
+    assert "Coordination error: Bad version" in ex_as_str
+    assert node_path in ex_as_str
+    assert client.get(node_path) == "value1"
+
+    client.set(node_path, "value2", 1)
+    assert client.get(node_path) == "value2"
+
+
+def test_set_without_version(client: KeeperClient):
+    node_path = "/test_set_without_version_node"
+    client.create(node_path, "value")
+    assert client.get(node_path) == "value"
+
+    client.set(node_path, "value1")
+    assert client.get(node_path) == "value1"
+
+    client.set(node_path, "value2")
+    assert client.get(node_path) == "value2"
+
+
+def test_quoted_argument_parsing(client: KeeperClient):
+    node_path = "/test_quoted_argument_parsing_node"
+    client.create(node_path, "value")
+
+    client.execute_query(f"set '{node_path}' 'value1 with some whitespace'")
+    assert client.get(node_path) == "value1 with some whitespace"
+
+    client.execute_query(f"set '{node_path}' 'value2 with some whitespace' 1")
+    assert client.get(node_path) == "value2 with some whitespace"
+
+    client.execute_query(f"set '{node_path}' \"value3 with some whitespace\"")
+    assert client.get(node_path) == "value3 with some whitespace"
+
+    client.execute_query(f"set '{node_path}' \"value4 with some whitespace\" 3")
+    assert client.get(node_path) == "value4 with some whitespace"