(accumulo) branch 2.1 updated: Setting empty property value no longer deletes property (#4731)

[ddanielr]

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

ddanielr pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git


The following commit(s) were added to refs/heads/2.1 by this push:
     new ff9a3f7442 Setting empty property value no longer deletes property 
(#4731)
ff9a3f7442 is described below

commit ff9a3f744206ebf6d8ff7dec9c869fe757c07c04
Author: Mark Owens <jmar...@apache.org>
AuthorDate: Tue Jul 23 16:27:04 2024 -0400

    Setting empty property value no longer deletes property (#4731)
    
    Modifed ManagerServiceClientHandler alterProperty method so it no
    longer removes/deletes a property when set to null or empty string.
    
    Added configPropertyTest integration tests to ShellIT.
---
 .../manager/ManagerClientServiceHandler.java       |  7 +++-
 .../org/apache/accumulo/test/shell/ShellIT.java    | 49 ++++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git 
a/server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java
 
b/server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java
index 1a8d0d0cac..ee914703cc 100644
--- 
a/server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java
+++ 
b/server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java
@@ -555,10 +555,13 @@ public class ManagerClientServiceHandler implements 
ManagerClientService.Iface {
     }
 
     try {
-      if (value == null || value.isEmpty()) {
+      if (op == TableOperation.REMOVE_PROPERTY) {
         PropUtil.removeProperties(manager.getContext(),
             TablePropKey.of(manager.getContext(), tableId), List.of(property));
-      } else {
+      } else if (op == TableOperation.SET_PROPERTY) {
+        if (value == null || value.isEmpty()) {
+          value = "";
+        }
         PropUtil.setProperties(manager.getContext(), 
TablePropKey.of(manager.getContext(), tableId),
             Map.of(property, value));
       }
diff --git a/test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java 
b/test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java
index 650ebac7c3..8e3ef9e867 100644
--- a/test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java
@@ -31,6 +31,7 @@ import java.nio.file.Files;
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
 import java.util.List;
@@ -475,6 +476,54 @@ public class ShellIT extends SharedMiniClusterBase {
     exec("deletetable t -f", true, "Table: [t] has been deleted");
   }
 
+  @Test
+  void configPropertyTest() throws IOException {
+    final String table = "testtable";
+    final String filterProperty = "config -t " + table + " -f ";
+    final String setProperty = "config -t " + table + " -s ";
+    List<String> expectedStrings = new ArrayList<>();
+
+    try {
+      exec("createtable " + table);
+
+      exec(filterProperty + "table.iterator.scan.vers.opt.maxVersions", true,
+          "table      | table.iterator.scan.vers.opt.maxVersions .. | 1\n");
+      // set to new value and verify results
+      exec(setProperty + "table.iterator.scan.vers.opt.maxVersions=2", true);
+      exec(filterProperty + "table.iterator.scan.vers.opt.maxVersions", true,
+          "table      | table.iterator.scan.vers.opt.maxVersions .. | 2\n");
+      // set to empty string and verify property still exists and is not 
deleted
+      exec(setProperty + "table.iterator.scan.vers.opt.maxVersions=", true);
+      exec(filterProperty + "table.iterator.scan.vers.opt.maxVersions", true,
+          "table      | table.iterator.scan.vers.opt.maxVersions .. | \n");
+
+      exec(filterProperty + "table.bloom.enabled", true,
+          "default    | table.bloom.enabled ....................... | 
false\n");
+      exec(setProperty + "table.bloom.enabled=true", true);
+      expectedStrings.add("default    | table.bloom.enabled 
....................... | false\n");
+      expectedStrings.add("table      |    @override 
.............................. | true\n");
+      execExpectList(filterProperty + "table.bloom.enabled", true, 
expectedStrings);
+      // can't set prop to empty value since type is Boolean
+      exec(setProperty + "table.bloom.enabled=failsSinceNotABoolean", false);
+
+      exec(filterProperty + "table.file.compress.type", true,
+          "default    | table.file.compress.type .................. | gz\n");
+      exec(setProperty + "table.file.compress.type=zippy", true);
+      expectedStrings.clear();
+      expectedStrings.add("default    | table.file.compress.type 
.................. | gz\n");
+      expectedStrings.add("table      |    @override 
.............................. | zippy");
+      execExpectList(filterProperty + "table.file.compress.type", true, 
expectedStrings);
+      exec(setProperty + "table.file.compress.type=", true);
+      expectedStrings.clear();
+      expectedStrings.add("default    | table.file.compress.type 
.................. | gz\n");
+      expectedStrings.add("table      |    @override 
.............................. | \n");
+      execExpectList(filterProperty + "table.file.compress.type", true, 
expectedStrings);
+
+    } finally {
+      exec("deletetable " + table + " -f", true);
+    }
+  }
+
   @Test
   void configTest() throws IOException {
     Shell.log.debug("Starting config property type test 
-------------------------");