[controller] Set default for is_abort_migration_cleanup to false wh…

…en null (#1494) Fix: default is_abort_migration_cleanup to false when null. Delete parseBooleanFromString method and replace its usage with parseBooleanOrThrow. Co-authored-by: Ran Wang <[email protected]>
linkedin · Feb 3, 2025 · b014e48 · b014e48
1 parent 9ab47e5
commit b014e48
Show file tree

Hide file tree

Showing 10 changed files with 104 additions and 42 deletions.
diff --git a/clients/venice-admin-tool/src/main/java/com/linkedin/venice/AdminTool.java b/clients/venice-admin-tool/src/main/java/com/linkedin/venice/AdminTool.java
@@ -841,8 +841,7 @@ private static void createNewStore(CommandLine cmd) throws Exception {
     String valueSchemaFile = getRequiredArgument(cmd, Arg.VALUE_SCHEMA, Command.NEW_STORE);
     String valueSchema = readFile(valueSchemaFile);
     String owner = getOptionalArgument(cmd, Arg.OWNER, "");
-    boolean isVsonStore =
-        Utils.parseBooleanFromString(getOptionalArgument(cmd, Arg.VSON_STORE, "false"), "isVsonStore");
+    boolean isVsonStore = Utils.parseBooleanOrThrow(getOptionalArgument(cmd, Arg.VSON_STORE, "false"), "isVsonStore");
     if (isVsonStore) {
       keySchema = VsonAvroSchemaAdapter.parse(keySchema).toString();
       valueSchema = VsonAvroSchemaAdapter.parse(valueSchema).toString();
@@ -1126,7 +1125,7 @@ private static void longParam(CommandLine cmd, Arg param, Consumer<Long> setter,
   }
 
   private static void booleanParam(CommandLine cmd, Arg param, Consumer<Boolean> setter, Set<Arg> argSet) {
-    genericParam(cmd, param, s -> Utils.parseBooleanFromString(s, param.toString()), setter, argSet);
+    genericParam(cmd, param, s -> Utils.parseBooleanOrThrow(s, param.toString()), setter, argSet);
   }
 
   private static void stringMapParam(
@@ -2536,8 +2535,7 @@ private static void createNewStoreWithAcl(CommandLine cmd) throws Exception {
     String valueSchema = readFile(valueSchemaFile);
     String aclPerms = getRequiredArgument(cmd, Arg.ACL_PERMS, Command.NEW_STORE);
     String owner = getOptionalArgument(cmd, Arg.OWNER, "");
-    boolean isVsonStore =
-        Utils.parseBooleanFromString(getOptionalArgument(cmd, Arg.VSON_STORE, "false"), "isVsonStore");
+    boolean isVsonStore = Utils.parseBooleanOrThrow(getOptionalArgument(cmd, Arg.VSON_STORE, "false"), "isVsonStore");
     if (isVsonStore) {
       keySchema = VsonAvroSchemaAdapter.parse(keySchema).toString();
       valueSchema = VsonAvroSchemaAdapter.parse(valueSchema).toString();

diff --git a/internal/venice-common/src/main/java/com/linkedin/venice/utils/Utils.java b/internal/venice-common/src/main/java/com/linkedin/venice/utils/Utils.java
@@ -304,24 +304,49 @@ public static long parseLongFromString(String value, String fieldName) {
   }
 
   /**
-   * Since {@link Boolean#parseBoolean(String)} does not throw exception and will always return 'false' for
-   * any string that are not equal to 'true', We validate the string by our own.
+   * Parses a boolean from a string, ensuring that only valid boolean values ("true" or "false")
+   * are accepted. Throws an exception if the value is null or invalid.
+   *
+   * @param value the string to parse
+   * @param fieldName the name of the field being validated
+   * @return the parsed boolean value
+   * @throws VeniceHttpException if the value is null or not "true" or "false"
    */
-  public static boolean parseBooleanFromString(String value, String fieldName) {
+  public static boolean parseBooleanOrThrow(String value, String fieldName) {
     if (value == null) {
       throw new VeniceHttpException(
           HttpStatus.SC_BAD_REQUEST,
-          fieldName + " must be a boolean, but value is null",
+          fieldName + " must be a boolean, but value is null.",
           ErrorType.BAD_REQUEST);
     }
-    if (value.equalsIgnoreCase("true") || value.equalsIgnoreCase("false")) {
-      return Boolean.parseBoolean(value);
-    } else {
+    return parseBoolean(value, fieldName);
+  }
+
+  /**
+   * Parses a boolean from a string, ensuring that only null and valid boolean values ("true" or "false")
+   * are accepted. Returns false if the value is null.
+   *
+   * @param value the string to parse
+   * @param fieldName the name of the field being validated
+   * @return the parsed boolean value, or false if the input is null
+   * @throws VeniceHttpException if the value is not "true" or "false"
+   */
+  public static boolean parseBooleanOrFalse(String value, String fieldName) {
+    return value != null && parseBoolean(value, fieldName);
+  }
+
+  /**
+   * Validates the boolean string, allowing only "true" or "false".
+   * Throws an exception if the value is invalid.
+   */
+  private static boolean parseBoolean(String value, String fieldName) {
+    if (!"true".equalsIgnoreCase(value) && !"false".equalsIgnoreCase(value)) {
       throw new VeniceHttpException(
           HttpStatus.SC_BAD_REQUEST,
-          fieldName + " must be a boolean, but value: " + value,
+          fieldName + " must be a boolean, but value: " + value + " is invalid.",
           ErrorType.BAD_REQUEST);
     }
+    return Boolean.parseBoolean(value);
   }
 
   /**

diff --git a/internal/venice-common/src/test/java/com/linkedin/venice/utils/UtilsTest.java b/internal/venice-common/src/test/java/com/linkedin/venice/utils/UtilsTest.java
@@ -4,11 +4,13 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertThrows;
 import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.expectThrows;
 import static org.testng.Assert.fail;
 
+import com.linkedin.venice.exceptions.ErrorType;
 import com.linkedin.venice.exceptions.VeniceException;
 import com.linkedin.venice.exceptions.VeniceHttpException;
 import com.linkedin.venice.meta.HybridStoreConfig;
@@ -82,7 +84,7 @@ public void testParseHostAndPortFromNodeIdentifier() {
   public void testGetDebugInfo() {
     Map<CharSequence, CharSequence> debugInfo = Utils.getDebugInfo();
     debugInfo.forEach((k, v) -> System.out.println(k + ": " + v));
-    Assert.assertFalse(debugInfo.isEmpty(), "debugInfo should not be empty.");
+    assertFalse(debugInfo.isEmpty(), "debugInfo should not be empty.");
     // N.B.: The "version" entry is not available in unit tests because of the way the classpath is built...
     String[] expectedKeys = { "path", "host", "pid", "user", "JDK major version" };
     assertTrue(
@@ -159,8 +161,8 @@ public void testDirectoryExists() throws Exception {
     Path filePath = Files.createTempFile(null, null);
     Path nonExistingPath = Paths.get(Utils.getUniqueTempPath());
     assertTrue(Utils.directoryExists(directoryPath.toString()));
-    Assert.assertFalse(Utils.directoryExists(filePath.toString()));
-    Assert.assertFalse(Utils.directoryExists(nonExistingPath.toString()));
+    assertFalse(Utils.directoryExists(filePath.toString()));
+    assertFalse(Utils.directoryExists(nonExistingPath.toString()));
     Files.delete(directoryPath);
     Files.delete(filePath);
   }
@@ -434,7 +436,7 @@ public void testParseDateTimeToEpoch() throws Exception {
   @Test
   public void testIsSeparateTopicRegion() {
     Assert.assertTrue(Utils.isSeparateTopicRegion("dc-0_sep"));
-    Assert.assertFalse(Utils.isSeparateTopicRegion("dc-0"));
+    assertFalse(Utils.isSeparateTopicRegion("dc-0"));
   }
 
   @Test
@@ -521,16 +523,55 @@ public Object[][] booleanParsingData() {
     };
   }
 
+  @DataProvider(name = "booleanOrFalseParsingData")
+  public Object[][] booleanOrFalseParsingData() {
+    return new Object[][] {
+        // Valid cases
+        { "true", "testField", true }, // Valid "true"
+        { "false", "testField", false }, // Valid "false"
+        { "TRUE", "testField", true }, // Valid case-insensitive "TRUE"
+        { "FALSE", "testField", false }, // Valid case-insensitive "FALSE"
+        { null, "testField", false }, // Null input
+
+        // Invalid cases
+        { "notABoolean", "testField", null }, // Invalid string
+        { "123", "testField", null }, // Non-boolean numeric string
+        { "", "testField", null }, // Empty string
+    };
+  }
+
   @Test(dataProvider = "booleanParsingData")
-  public void testParseBooleanFromString(String value, String fieldName, Boolean expectedResult) {
+  public void testParseBooleanOrThrow(String value, String fieldName, Boolean expectedResult) {
     if (expectedResult != null) {
       // For valid cases
-      boolean result = Utils.parseBooleanFromString(value, fieldName);
-      assertEquals((boolean) expectedResult, result, "Parsed boolean value does not match expected value.");
+      boolean result = Utils.parseBooleanOrThrow(value, fieldName);
+      assertEquals(result, (boolean) expectedResult, "Parsed boolean value does not match expected value.");
+      return;
+    }
+    VeniceHttpException e = expectThrows(VeniceHttpException.class, () -> Utils.parseBooleanOrThrow(value, fieldName));
+    assertEquals(e.getHttpStatusCode(), HttpStatus.SC_BAD_REQUEST, "Invalid status code.");
+    if (value == null) {
+      assertEquals(e.getMessage(), "Http Status 400 - testField must be a boolean, but value is null.");
+    } else {
+      assertEquals(
+          e.getMessage(),
+          "Http Status 400 - testField must be a boolean, but value: " + value + " is invalid.");
+    }
+    assertEquals(e.getErrorType(), ErrorType.BAD_REQUEST);
+  }
+
+  @Test(dataProvider = "booleanOrFalseParsingData")
+  public void testParseBooleanOrFalse(String value, String fieldName, Boolean expectedResult) {
+    // For valid cases
+    if (expectedResult != null) {
+      boolean result = Utils.parseBooleanOrFalse(value, fieldName);
+      assertEquals(result, (boolean) expectedResult, "Parsed boolean value does not match expected value.");
       return;
     }
-    VeniceHttpException e =
-        expectThrows(VeniceHttpException.class, () -> Utils.parseBooleanFromString(value, fieldName));
+    // For invalid cases
+    VeniceHttpException e = expectThrows(VeniceHttpException.class, () -> Utils.parseBooleanOrThrow(value, fieldName));
     assertEquals(e.getHttpStatusCode(), HttpStatus.SC_BAD_REQUEST, "Invalid status code.");
+    assertEquals(e.getMessage(), "Http Status 400 - testField must be a boolean, but value: " + value + " is invalid.");
+    assertEquals(e.getErrorType(), ErrorType.BAD_REQUEST);
   }
 }
diff --git a/...nice-controller/src/main/java/com/linkedin/venice/controller/server/ControllerRoutes.java b/...nice-controller/src/main/java/com/linkedin/venice/controller/server/ControllerRoutes.java
@@ -121,7 +121,7 @@ public Route updateKafkaTopicLogCompaction(Admin admin) {
     return updateKafkaTopicConfig(admin, adminRequest -> {
       AdminSparkServer.validateParams(adminRequest, UPDATE_KAFKA_TOPIC_LOG_COMPACTION.getParams(), admin);
       PubSubTopic topicName = pubSubTopicRepository.getTopic(adminRequest.queryParams(TOPIC));
-      boolean kafkaTopicLogCompactionEnabled = Utils.parseBooleanFromString(
+      boolean kafkaTopicLogCompactionEnabled = Utils.parseBooleanOrThrow(
           adminRequest.queryParams(KAFKA_TOPIC_LOG_COMPACTION_ENABLED),
           KAFKA_TOPIC_LOG_COMPACTION_ENABLED);
 

diff --git a/.../venice-controller/src/main/java/com/linkedin/venice/controller/server/CreateVersion.java b/.../venice-controller/src/main/java/com/linkedin/venice/controller/server/CreateVersion.java
@@ -82,20 +82,19 @@ protected static void extractOptionalParamsFromRequestTopicRequest(
     request.setPartitioners(httpRequest.queryParamOrDefault(PARTITIONERS, null));
 
     request.setSendStartOfPush(
-        Utils.parseBooleanFromString(httpRequest.queryParamOrDefault(SEND_START_OF_PUSH, "false"), SEND_START_OF_PUSH));
+        Utils.parseBooleanOrThrow(httpRequest.queryParamOrDefault(SEND_START_OF_PUSH, "false"), SEND_START_OF_PUSH));
 
     request.setSorted(
-        Utils.parseBooleanFromString(
-            httpRequest.queryParamOrDefault(PUSH_IN_SORTED_ORDER, "false"),
-            PUSH_IN_SORTED_ORDER));
+        Utils
+            .parseBooleanOrThrow(httpRequest.queryParamOrDefault(PUSH_IN_SORTED_ORDER, "false"), PUSH_IN_SORTED_ORDER));
 
     request.setWriteComputeEnabled(
-        Utils.parseBooleanFromString(
+        Utils.parseBooleanOrThrow(
             httpRequest.queryParamOrDefault(IS_WRITE_COMPUTE_ENABLED, "false"),
             IS_WRITE_COMPUTE_ENABLED));
 
     request.setSeparateRealTimeTopicEnabled(
-        Utils.parseBooleanFromString(
+        Utils.parseBooleanOrThrow(
             httpRequest.queryParamOrDefault(SEPARATE_REAL_TIME_TOPIC_ENABLED, "false"),
             SEPARATE_REAL_TIME_TOPIC_ENABLED));
 
@@ -109,7 +108,7 @@ protected static void extractOptionalParamsFromRequestTopicRequest(
      * Version level override to defer marking this new version to the serving version post push completion.
      */
     request.setDeferVersionSwap(
-        Utils.parseBooleanFromString(httpRequest.queryParamOrDefault(DEFER_VERSION_SWAP, "false"), DEFER_VERSION_SWAP));
+        Utils.parseBooleanOrThrow(httpRequest.queryParamOrDefault(DEFER_VERSION_SWAP, "false"), DEFER_VERSION_SWAP));
 
     request.setTargetedRegions(httpRequest.queryParamOrDefault(TARGETED_REGIONS, null));
 

diff --git a/...ce-controller/src/main/java/com/linkedin/venice/controller/server/DataRecoveryRoutes.java b/...ce-controller/src/main/java/com/linkedin/venice/controller/server/DataRecoveryRoutes.java
@@ -48,12 +48,11 @@ public void internalHandle(Request request, ControllerResponse veniceResponse) {
         int version = Utils.parseIntFromString(request.queryParams(VERSION), VERSION);
         String sourceFabric = request.queryParams(SOURCE_FABRIC);
         String destinationFabric = request.queryParams(FABRIC);
-        boolean copyAllVersionConfigs = Utils.parseBooleanFromString(
+        boolean copyAllVersionConfigs = Utils.parseBooleanOrThrow(
             request.queryParams(DATA_RECOVERY_COPY_ALL_VERSION_CONFIGS),
             DATA_RECOVERY_COPY_ALL_VERSION_CONFIGS);
-        boolean sourceVersionIncluded = Utils.parseBooleanFromString(
-            request.queryParams(SOURCE_FABRIC_VERSION_INCLUDED),
-            SOURCE_FABRIC_VERSION_INCLUDED);
+        boolean sourceVersionIncluded = Utils
+            .parseBooleanOrThrow(request.queryParams(SOURCE_FABRIC_VERSION_INCLUDED), SOURCE_FABRIC_VERSION_INCLUDED);
         Optional<Version> sourceVersion;
         if (sourceVersionIncluded) {
           Version sourceVersionObject = null;

diff --git a/...oller/src/main/java/com/linkedin/venice/controller/server/RoutersClusterConfigRoutes.java b/...oller/src/main/java/com/linkedin/venice/controller/server/RoutersClusterConfigRoutes.java
@@ -37,7 +37,7 @@ public void internalHandle(Request request, ControllerResponse veniceResponse) {
         AdminSparkServer.validateParams(request, ENABLE_THROTTLING.getParams(), admin);
         String clusterName = request.queryParams(CLUSTER);
         veniceResponse.setCluster(clusterName);
-        boolean status = Utils.parseBooleanFromString(request.queryParams(STATUS), "enableThrottling");
+        boolean status = Utils.parseBooleanOrThrow(request.queryParams(STATUS), "enableThrottling");
         admin.updateRoutersClusterConfig(
             clusterName,
             Optional.of(status),
@@ -62,7 +62,7 @@ public void internalHandle(Request request, ControllerResponse veniceResponse) {
         AdminSparkServer.validateParams(request, ENABLE_MAX_CAPACITY_PROTECTION.getParams(), admin);
         String clusterName = request.queryParams(CLUSTER);
         veniceResponse.setCluster(clusterName);
-        boolean status = Utils.parseBooleanFromString(request.queryParams(STATUS), "enableMaxCapacityProtection");
+        boolean status = Utils.parseBooleanOrThrow(request.queryParams(STATUS), "enableMaxCapacityProtection");
         admin.updateRoutersClusterConfig(
             clusterName,
             Optional.empty(),
@@ -87,7 +87,7 @@ public void internalHandle(Request request, ControllerResponse veniceResponse) {
         AdminSparkServer.validateParams(request, ENABLE_QUOTA_REBALANCED.getParams(), admin);
         String clusterName = request.queryParams(CLUSTER);
         veniceResponse.setCluster(clusterName);
-        boolean status = Utils.parseBooleanFromString(request.queryParams(STATUS), "enableQuotaRebalance");
+        boolean status = Utils.parseBooleanOrThrow(request.queryParams(STATUS), "enableQuotaRebalance");
         int expectedRouterCount =
             Utils.parseIntFromString(request.queryParams(EXPECTED_ROUTER_COUNT), "expectedRouterCount");
         admin.updateRoutersClusterConfig(

diff --git a/...venice-controller/src/main/java/com/linkedin/venice/controller/server/SkipAdminRoute.java b/...venice-controller/src/main/java/com/linkedin/venice/controller/server/SkipAdminRoute.java
@@ -39,7 +39,7 @@ public Route skipAdminMessage(Admin admin) {
         AdminSparkServer.validateParams(request, SKIP_ADMIN.getParams(), admin);
         responseObject.setCluster(request.queryParams(CLUSTER));
         long offset = Utils.parseLongFromString(request.queryParams(OFFSET), OFFSET);
-        boolean skipDIV = Utils.parseBooleanFromString(request.queryParams(SKIP_DIV), SKIP_DIV);
+        boolean skipDIV = Utils.parseBooleanOrThrow(request.queryParams(SKIP_DIV), SKIP_DIV);
         admin.skipAdminMessage(responseObject.getCluster(), offset, skipDIV);
       } catch (Throwable e) {
         responseObject.setError(e);

diff --git a/...s/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java b/...s/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java
@@ -520,7 +520,7 @@ public void internalHandle(Request request, TrackableControllerResponse veniceRe
           String clusterName = request.queryParams(CLUSTER);
           String storeName = request.queryParams(NAME);
           boolean abortMigratingStore =
-              Utils.parseBooleanFromString(request.queryParams(IS_ABORT_MIGRATION_CLEANUP), IS_ABORT_MIGRATION_CLEANUP);
+              Utils.parseBooleanOrFalse(request.queryParams(IS_ABORT_MIGRATION_CLEANUP), IS_ABORT_MIGRATION_CLEANUP);
           veniceResponse.setCluster(clusterName);
           veniceResponse.setName(storeName);
 
@@ -696,7 +696,7 @@ public void internalHandle(Request request, ControllerResponse veniceResponse) {
         String cluster = request.queryParams(CLUSTER);
         String storeName = request.queryParams(NAME);
         String operation = request.queryParams(OPERATION);
-        boolean status = Utils.parseBooleanFromString(request.queryParams(STATUS), "storeAccessStatus");
+        boolean status = Utils.parseBooleanOrThrow(request.queryParams(STATUS), "storeAccessStatus");
 
         veniceResponse.setCluster(cluster);
         veniceResponse.setName(storeName);
@@ -812,7 +812,7 @@ public void internalHandle(Request request, ControllerResponse veniceResponse) {
 
         String cluster = request.queryParams(CLUSTER);
         boolean enableActiveActiveReplicationForCluster =
-            Utils.parseBooleanFromString(request.queryParams(STATUS), STATUS);
+            Utils.parseBooleanOrThrow(request.queryParams(STATUS), STATUS);
         String regionsFilterParams = request.queryParamOrDefault(REGIONS_FILTER, null);
 
         admin.configureActiveActiveReplication(

diff --git a/...enice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRoutesTest.java b/...enice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRoutesTest.java
@@ -101,7 +101,6 @@ public void testDeleteStore() throws Exception {
     Request request = mock(Request.class);
     doReturn(TEST_CLUSTER).when(request).queryParams(eq(ControllerApiConstants.CLUSTER));
     doReturn(TEST_STORE_NAME).when(request).queryParams(eq(ControllerApiConstants.NAME));
-    doReturn("true").when(request).queryParams(eq(ControllerApiConstants.IS_ABORT_MIGRATION_CLEANUP));
 
     Route deleteStoreRoute = new StoresRoutes(false, Optional.empty(), pubSubTopicRepository).deleteStore(mockAdmin);
     TrackableControllerResponse trackableControllerResponse = ObjectMapperFactory.getInstance()
@@ -112,6 +111,7 @@ public void testDeleteStore() throws Exception {
     Assert.assertEquals(trackableControllerResponse.getCluster(), TEST_CLUSTER);
     Assert.assertEquals(trackableControllerResponse.getName(), TEST_STORE_NAME);
 
+    doReturn("true").when(request).queryParams(eq(ControllerApiConstants.IS_ABORT_MIGRATION_CLEANUP));
     String errMessage = "Store " + TEST_STORE_NAME + "'s migrating flag is false. Not safe to delete a store "
         + "that is assumed to be migrating without the migrating flag setup as true.";
     doThrow(new VeniceException(errMessage, INVALID_CONFIG)).when(mockAdmin)