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

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


The following commit(s) were added to refs/heads/master by this push:
     new 6741396  Fix the toggle table state API (#4509)
6741396 is described below

commit 674139674b0e72188b1b49c48f687303d3f7a352
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Thu Aug 8 17:55:03 2019 -0700

    Fix the toggle table state API (#4509)
    
    Currently toggle table state API is not calling the correct method to drop 
the table
    Re-write toggle table state API and move it into PinotHelixResourceManager
---
 .../api/resources/PinotTableRestletResource.java   | 67 ++++++-----------
 .../helix/core/PinotHelixResourceManager.java      | 86 +++++++++-------------
 2 files changed, 59 insertions(+), 94 deletions(-)

diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
index f22a1f2..6ac6912 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
@@ -60,7 +60,6 @@ import 
org.apache.pinot.common.utils.CommonConstants.Helix.TableType;
 import org.apache.pinot.common.utils.JsonUtils;
 import org.apache.pinot.controller.ControllerConf;
 import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
-import org.apache.pinot.controller.helix.core.PinotResourceManagerResponse;
 import 
org.apache.pinot.controller.helix.core.rebalance.RebalanceUserConfigConstants;
 import org.apache.pinot.core.util.ReplicationUtils;
 import org.slf4j.LoggerFactory;
@@ -207,33 +206,35 @@ public class PinotTableRestletResource {
       if (stateStr == null) {
         return listTableConfigs(tableName, tableTypeStr);
       }
-      Constants.validateState(stateStr);
+
+      StateType stateType = Constants.validateState(stateStr);
+      TableType tableType = Constants.validateTableType(tableTypeStr);
+
       ArrayNode ret = JsonUtils.newArrayNode();
       boolean tableExists = false;
 
-      if ((tableTypeStr == null || 
CommonConstants.Helix.TableType.OFFLINE.name().equalsIgnoreCase(tableTypeStr))
-          && _pinotHelixResourceManager.hasOfflineTable(tableName)) {
+      if (tableType != TableType.REALTIME && 
_pinotHelixResourceManager.hasOfflineTable(tableName)) {
         String offlineTableName = 
TableNameBuilder.OFFLINE.tableNameWithType(tableName);
         ObjectNode offline = JsonUtils.newObjectNode();
         tableExists = true;
 
         offline.put(FileUploadPathProvider.TABLE_NAME, offlineTableName);
         offline.set(FileUploadPathProvider.STATE,
-            JsonUtils.objectToJsonNode(toggleTableState(offlineTableName, 
stateStr)));
+            
JsonUtils.objectToJsonNode(_pinotHelixResourceManager.toggleTableState(offlineTableName,
 stateType)));
         ret.add(offline);
       }
 
-      if ((tableTypeStr == null || 
CommonConstants.Helix.TableType.REALTIME.name().equalsIgnoreCase(tableTypeStr))
-          && _pinotHelixResourceManager.hasRealtimeTable(tableName)) {
-        String realTimeTableName = 
TableNameBuilder.REALTIME.tableNameWithType(tableName);
-        ObjectNode realTime = JsonUtils.newObjectNode();
+      if (tableType != TableType.OFFLINE && 
_pinotHelixResourceManager.hasRealtimeTable(tableName)) {
+        String realtimeTableName = 
TableNameBuilder.REALTIME.tableNameWithType(tableName);
+        ObjectNode realtime = JsonUtils.newObjectNode();
         tableExists = true;
 
-        realTime.put(FileUploadPathProvider.TABLE_NAME, realTimeTableName);
-        realTime.set(FileUploadPathProvider.STATE,
-            JsonUtils.objectToJsonNode(toggleTableState(realTimeTableName, 
stateStr)));
-        ret.add(realTime);
+        realtime.put(FileUploadPathProvider.TABLE_NAME, realtimeTableName);
+        realtime.set(FileUploadPathProvider.STATE,
+            
JsonUtils.objectToJsonNode(_pinotHelixResourceManager.toggleTableState(realtimeTableName,
 stateType)));
+        ret.add(realtime);
       }
+
       if (tableExists) {
         return ret.toString();
       } else {
@@ -252,19 +253,19 @@ public class PinotTableRestletResource {
   public SuccessResponse deleteTable(
       @ApiParam(value = "Name of the table to delete", required = true) 
@PathParam("tableName") String tableName,
       @ApiParam(value = "realtime|offline") @QueryParam("type") String 
tableTypeStr) {
+    TableType tableType = Constants.validateTableType(tableTypeStr);
+
     List<String> tablesDeleted = new LinkedList<>();
     try {
-      if ((tableTypeStr == null || 
tableTypeStr.equalsIgnoreCase(CommonConstants.Helix.TableType.OFFLINE.name()))
-          && !TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+      if (tableType != TableType.REALTIME && 
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
         _pinotHelixResourceManager.deleteOfflineTable(tableName);
         
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
       }
-      if ((tableTypeStr == null || 
tableTypeStr.equalsIgnoreCase(CommonConstants.Helix.TableType.REALTIME.name()))
-          && !TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+      if (tableType != TableType.OFFLINE && 
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
         _pinotHelixResourceManager.deleteRealtimeTable(tableName);
         
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
       }
-      return new SuccessResponse("Table deleted " + tablesDeleted);
+      return new SuccessResponse("Tables: " + tablesDeleted + " deleted");
     } catch (Exception e) {
       throw new ControllerApplicationException(LOGGER, e.getMessage(), 
Response.Status.INTERNAL_SERVER_ERROR, e);
     }
@@ -335,21 +336,6 @@ public class PinotTableRestletResource {
     }
   }
 
-  // TODO: move this method into PinotHelixResourceManager
-  private PinotResourceManagerResponse toggleTableState(String tableName, 
String state) {
-    if (StateType.ENABLE.name().equalsIgnoreCase(state)) {
-      return _pinotHelixResourceManager.toggleTableState(tableName, true);
-    } else if (StateType.DISABLE.name().equalsIgnoreCase(state)) {
-      return _pinotHelixResourceManager.toggleTableState(tableName, false);
-    } else if (StateType.DROP.name().equalsIgnoreCase(state)) {
-      return _pinotHelixResourceManager.dropTable(tableName);
-    } else {
-      String errorMessage = "Invalid state: " + state + ", must be one of 
{enable|disable|drop}";
-      LOGGER.info(errorMessage);
-      return PinotResourceManagerResponse.failure(errorMessage);
-    }
-  }
-
   private void ensureMinReplicas(TableConfig tableConfig) {
     // For self-serviced cluster, ensure that the tables are created with at 
least min replication factor irrespective
     // of table configuration value
@@ -462,14 +448,10 @@ public class PinotTableRestletResource {
   public String rebalance(
       @ApiParam(value = "Name of the table to rebalance") @Nonnull 
@PathParam("tableName") String tableName,
       @ApiParam(value = "offline|realtime") @Nonnull @QueryParam("type") 
String tableType,
-      @ApiParam(value = "true if rebalancer should be run in dryRun mode, 
false otherwise")
-      @Nonnull @DefaultValue("true") @QueryParam("dryrun") Boolean dryRun,
-      @ApiParam(value = "true if consuming segments should be considered for 
rebalancing realtime tables, false otherwise")
-      @DefaultValue("false") @QueryParam("includeConsuming") Boolean 
includeConsuming,
-      @ApiParam(value = "true if downtime is acceptable during rebalance, 
false otherwise")
-      @DefaultValue("false") @QueryParam("downtime") Boolean downtime,
-      @ApiParam(value = "minimum number of replicas to keep alive during 
rebalance(if downtime is false)")
-      @DefaultValue("1") @QueryParam("minAvailableReplicas") Integer 
minAvailableReplicas) {
+      @ApiParam(value = "true if rebalancer should be run in dryRun mode, 
false otherwise") @Nonnull @DefaultValue("true") @QueryParam("dryrun") Boolean 
dryRun,
+      @ApiParam(value = "true if consuming segments should be considered for 
rebalancing realtime tables, false otherwise") @DefaultValue("false") 
@QueryParam("includeConsuming") Boolean includeConsuming,
+      @ApiParam(value = "true if downtime is acceptable during rebalance, 
false otherwise") @DefaultValue("false") @QueryParam("downtime") Boolean 
downtime,
+      @ApiParam(value = "minimum number of replicas to keep alive during 
rebalance(if downtime is false)") @DefaultValue("1") 
@QueryParam("minAvailableReplicas") Integer minAvailableReplicas) {
 
     if (tableType != null && 
!EnumUtils.isValidEnum(CommonConstants.Helix.TableType.class, 
tableType.toUpperCase())) {
       throw new ControllerApplicationException(LOGGER, "Illegal table type " + 
tableType, Response.Status.BAD_REQUEST);
@@ -507,8 +489,7 @@ public class PinotTableRestletResource {
           }
         });
         result = new RebalanceResult();
-        result
-            .setStatus("Rebalance for table " + tableName + " in progress. 
Check controller logs for updates.");
+        result.setStatus("Rebalance for table " + tableName + " in progress. 
Check controller logs for updates.");
       }
     } catch (TableNotFoundException e) {
       throw new ControllerApplicationException(LOGGER, e.getMessage(), 
Response.Status.NOT_FOUND);
diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
index b6d4b03..3c8eb62 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
@@ -100,6 +100,7 @@ import org.apache.pinot.common.utils.retry.RetryPolicies;
 import org.apache.pinot.common.utils.retry.RetryPolicy;
 import org.apache.pinot.controller.ControllerConf;
 import org.apache.pinot.controller.api.pojos.Instance;
+import org.apache.pinot.controller.api.resources.StateType;
 import 
org.apache.pinot.controller.helix.core.realtime.PinotLLCRealtimeSegmentManager;
 import 
org.apache.pinot.controller.helix.core.rebalance.RebalanceSegmentStrategy;
 import 
org.apache.pinot.controller.helix.core.rebalance.RebalanceSegmentStrategyFactory;
@@ -1397,8 +1398,8 @@ public class PinotHelixResourceManager {
         }, RetryPolicies.exponentialBackoffRetryPolicy(5, 500L, 2.0f));
   }
 
-  public void deleteOfflineTable(String rawTableName) {
-    String offlineTableName = 
TableNameBuilder.OFFLINE.tableNameWithType(rawTableName);
+  public void deleteOfflineTable(String tableName) {
+    String offlineTableName = 
TableNameBuilder.OFFLINE.tableNameWithType(tableName);
     LOGGER.info("Deleting table {}: Start", offlineTableName);
 
     // Remove the table from brokerResource
@@ -1476,60 +1477,43 @@ public class PinotHelixResourceManager {
   }
 
   /**
-   * Toggle the status of the table between OFFLINE and ONLINE.
-   *
-   * @param tableName: Name of the table for which to toggle the status.
-   * @param status: True for ONLINE and False for OFFLINE.
-   * @return
-   */
-  public PinotResourceManagerResponse toggleTableState(String tableName, 
boolean status) {
-    if 
(!_helixAdmin.getResourcesInCluster(_helixClusterName).contains(tableName)) {
-      return PinotResourceManagerResponse.failure("Table " + tableName + " not 
found");
-    }
-    _helixAdmin.enableResource(_helixClusterName, tableName, status);
-
-    // If enabling a resource, also reset segments in error state for that 
resource
-    boolean resetSuccessful = false;
-    if (status) {
-      try {
-        _helixAdmin.resetResource(_helixClusterName, 
Collections.singletonList(tableName));
-        resetSuccessful = true;
-      } catch (HelixException e) {
-        LOGGER.warn("Caught exception while resetting resource {}, ignoring.", 
e, tableName);
-      }
-    }
-
-    return (status) ? PinotResourceManagerResponse
-        .success("Table " + tableName + " enabled (reset success = " + 
resetSuccessful + ")")
-        : PinotResourceManagerResponse.success("Table " + tableName + " 
disabled");
-  }
-
-  /**
-   * Drop the table from helix cluster.
-   *
-   * @param tableName: Name of table to be dropped.
-   * @return
+   * Toggles the state (ONLINE|OFFLINE|DROP) of the given table.
    */
-  public PinotResourceManagerResponse dropTable(String tableName) {
-    if 
(!_helixAdmin.getResourcesInCluster(_helixClusterName).contains(tableName)) {
-      return PinotResourceManagerResponse.failure("Table " + tableName + " not 
found");
-    }
-
-    if (getSegmentsFor(tableName).size() != 0) {
-      return PinotResourceManagerResponse.failure("Table " + tableName + " has 
segments, drop them first");
+  public PinotResourceManagerResponse toggleTableState(String 
tableNameWithType, StateType stateType) {
+    if (!hasTable(tableNameWithType)) {
+      return PinotResourceManagerResponse.failure("Table: " + 
tableNameWithType + " not found");
+    }
+    switch (stateType) {
+      case ENABLE:
+        _helixAdmin.enableResource(_helixClusterName, tableNameWithType, true);
+        // Reset segments in ERROR state
+        boolean resetSuccessful = false;
+        try {
+          _helixAdmin.resetResource(_helixClusterName, 
Collections.singletonList(tableNameWithType));
+          resetSuccessful = true;
+        } catch (HelixException e) {
+          LOGGER.warn("Caught exception while resetting resource: {}", 
tableNameWithType, e);
+        }
+        return PinotResourceManagerResponse
+            .success("Table: " + tableNameWithType + " enabled (reset success 
= " + resetSuccessful + ")");
+      case DISABLE:
+        _helixAdmin.enableResource(_helixClusterName, tableNameWithType, 
false);
+        return PinotResourceManagerResponse.success("Table: " + 
tableNameWithType + " disabled");
+      case DROP:
+        TableType tableType = 
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+        if (tableType == TableType.OFFLINE) {
+          deleteOfflineTable(tableNameWithType);
+        } else {
+          deleteRealtimeTable(tableNameWithType);
+        }
+        return PinotResourceManagerResponse.success("Table: " + 
tableNameWithType + " dropped");
+      default:
+        throw new IllegalStateException();
     }
-
-    _helixAdmin.dropResource(_helixClusterName, tableName);
-
-    // remove from property store
-    
ZKMetadataProvider.removeResourceSegmentsFromPropertyStore(getPropertyStore(), 
tableName);
-    
ZKMetadataProvider.removeResourceConfigFromPropertyStore(getPropertyStore(), 
tableName);
-
-    return PinotResourceManagerResponse.success("Table " + tableName + " 
dropped");
   }
 
   private Set<String> getAllInstancesForTable(String tableName) {
-    Set<String> instanceSet = new HashSet<String>();
+    Set<String> instanceSet = new HashSet<>();
     IdealState tableIdealState = 
_helixAdmin.getResourceIdealState(_helixClusterName, tableName);
     for (String partition : tableIdealState.getPartitionSet()) {
       instanceSet.addAll(tableIdealState.getInstanceSet(partition));


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to