mcvsubbu commented on a change in pull request #4806: Refactor segments admin 
rest APIs
URL: https://github.com/apache/incubator-pinot/pull/4806#discussion_r346610793
 
 

 ##########
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
 ##########
 @@ -19,255 +19,409 @@
 package org.apache.pinot.controller.api.resources;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
-import com.fasterxml.jackson.databind.node.ArrayNode;
-import com.fasterxml.jackson.databind.node.ObjectNode;
 import io.swagger.annotations.Api;
 import io.swagger.annotations.ApiOperation;
 import io.swagger.annotations.ApiParam;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
-import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.inject.Inject;
+import javax.ws.rs.DELETE;
 import javax.ws.rs.Encoded;
 import javax.ws.rs.GET;
 import javax.ws.rs.POST;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
 import javax.ws.rs.Produces;
 import javax.ws.rs.QueryParam;
+import javax.ws.rs.WebApplicationException;
 import javax.ws.rs.core.MediaType;
-import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
 import org.apache.helix.ZNRecord;
 import org.apache.helix.store.zk.ZkHelixPropertyStore;
 import org.apache.pinot.common.config.TableNameBuilder;
 import org.apache.pinot.common.metadata.ZKMetadataProvider;
 import org.apache.pinot.common.metadata.segment.OfflineSegmentZKMetadata;
 import org.apache.pinot.common.metadata.segment.RealtimeSegmentZKMetadata;
-import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.common.utils.CommonConstants.Helix.TableType;
 import org.apache.pinot.common.utils.JsonUtils;
+import org.apache.pinot.common.utils.URIUtils;
 import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
 import org.apache.pinot.controller.helix.core.PinotResourceManagerResponse;
-import org.apache.pinot.common.utils.URIUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import static org.apache.pinot.controller.api.resources.Constants.TABLE_NAME;
-import static 
org.apache.pinot.controller.api.resources.FileUploadPathProvider.STATE;
-
 
 /**
- * Current URI Mappings:
+ * Segment admin rest APIs:
  * <ul>
  *   <li>
- *     "/tables/{tableName}/segments/{segmentName}":
- *     "/tables/{tableName}/segments/{segmentName}/metadata":
- *     Get segment metadata for a given segment
- *   </li>
- *   <li>
- *     "/tables/{tableName}/segments":
- *     "/tables/{tableName}/segments/metadata":
- *     List segment metadata for a given table
- *   </li>
- *   <li>
- *      "/tables/{tableName}/segments/crc":
- *      Get crc information for a given table
+ *     GET requests:
+ *     <ul>
+ *       <li>"/segments/{tableName}": get the name of all segments</li>
+ *       <li>"/segments/{tableName}/servers": get a map from server to 
segments hosted by the server</li>
+ *       <li>"/segments/{tableName}/crc": get a map from segment to CRC of the 
segment</li>
+ *       <li>"/segments/{tableName}/{segmentName}/metadata: get the metadata 
for a segment</li>
+ *     </ul>
  *   </li>
  *   <li>
- *     "/tables/{tableName}/segments/{segmentName}?state={state}":
- *     Change the state of the segment to specified {state} 
(enable|disable|drop)
+ *     POST requests:
+ *     <ul>
+ *       <li>"/segments/{tableName}/{segmentName}/reload": reload a 
segment</li>
+ *       <li>"/segments/{tableName}/reload": reload all segments</li>
+ *     </ul>
  *   </li>
  *   <li>
- *     "/tables/{tableName}/segments?state={state}":
- *     Change the state of all segments of the table to specified {state} 
(enable|disable|drop)
+ *     DELETE requests:
+ *     <ul>
+ *       <li>"/segments/{tableName}/{segmentName}": delete a segment</li>
+ *       <li>"/segments/{tableName}: delete all segments</li>
+ *     </ul>
  *   </li>
  *   <li>
- *     "/tables/{tableName}/segments/{segmentName}/reload":
- *     Reload the segment
+ *     All requests (except for crc fetch which only apply to OFFLINE table) 
take an optional query parameter "type"
+ *     (OFFLINE or REALTIME) for table type. The request will be performed to 
tables that match the table name and type,
+ *     e.g. "foobar_OFFLINE" matches ("foobar_OFFLINE", null), 
("foobar_OFFLINE", OFFLINE), ("foobar", null),
+ *     ("foobar", OFFLINE), but does not match ("foo", null), 
("foobar_REALTIME", null), ("foobar_REALTIME", OFFLINE),
+ *     ("foobar_OFFLINE", REALTIME).
  *   </li>
  *   <li>
- *     "/tables/{tableName}/segments/reload":
- *     Reload all segments of the table
+ *     Deprecated APIs:
+ *     <ul>
+ *       <li>"GET /tables/{tableName}/segments"</li>
+ *       <li>"GET /tables/{tableName}/segments/metadata"</li>
+ *       <li>"GET /tables/{tableName}/segments/crc"</li>
+ *       <li>"GET /tables/{tableName}/segments/{segmentName}"</li>
+ *       <li>"GET /tables/{tableName}/segments/{segmentName}/metadata"</li>
+ *       <li>"GET /tables/{tableName}/segments/{segmentName}/reload"</li>
+ *       <li>"POST /tables/{tableName}/segments/{segmentName}/reload"</li>
+ *       <li>"GET /tables/{tableName}/segments/reload"</li>
+ *       <li>"POST /tables/{tableName}/segments/reload"</li>
+ *     </ul>
  *   </li>
  * </ul>
- *
- * {@inheritDoc}
  */
-
-@Api(tags = Constants.TABLE_TAG)
+@Api(tags = Constants.SEGMENT_TAG)
 @Path("/")
 public class PinotSegmentRestletResource {
-  public static Logger LOGGER = 
LoggerFactory.getLogger(PinotSegmentRestletResource.class);
-  public static final Response.Status BAD_REQUEST = 
Response.Status.BAD_REQUEST;
-  public static final Response.Status INTERNAL_ERROR = 
Response.Status.INTERNAL_SERVER_ERROR;
-
-  private static final long _offlineToOnlineTimeoutInseconds = 5L;
+  private static Logger LOGGER = 
LoggerFactory.getLogger(PinotSegmentRestletResource.class);
 
   @Inject
   PinotHelixResourceManager _pinotHelixResourceManager;
 
   @GET
-  @Path("tables/{tableName}/segments")
   @Produces(MediaType.APPLICATION_JSON)
-  @ApiOperation(value = "Lists metadata or toggles segment states", notes = 
"Toggles segment states if 'state' is specified in query param, otherwise lists 
metadata")
-  // TODO: when state is not specified, this method returns the map from 
server to segments instead of segment metadata
-  public String toggleStateOrListMetadataForAllSegments(
+  @Path("/segments/{tableName}")
+  @ApiOperation(value = "List all segments", notes = "List all segments")
+  public List<Map<TableType, List<String>>> getSegments(
       @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName,
-      @ApiParam(value = "enable|disable|drop", required = false) 
@QueryParam("state") String stateStr,
-      @ApiParam(value = "realtime|offline", required = false) 
@QueryParam("type") String tableTypeStr) {
-    CommonConstants.Helix.TableType tableType = 
Constants.validateTableType(tableTypeStr);
-    StateType state = Constants.validateState(stateStr);
+      @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String 
tableTypeStr) {
+    List<String> tableNamesWithType =
+        getExistingTableNamesWithType(tableName, 
Constants.validateTableType(tableTypeStr));
+    List<Map<TableType, List<String>>> resultList = new 
ArrayList<>(tableNamesWithType.size());
+    for (String tableNameWithType : tableNamesWithType) {
+      TableType tableType = 
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+      List<String> segments = 
_pinotHelixResourceManager.getSegmentsFor(tableNameWithType);
+      resultList.add(Collections.singletonMap(tableType, segments));
+    }
+    return resultList;
+  }
 
-    if (stateStr == null) {
-      return getInstanceToSegmentsMap(tableName, tableType);
+  @GET
+  @Path("segments/{tableName}/servers")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Get a map from server to segments hosted by the 
server", notes = "Get a map from server to segments hosted by the server")
+  public List<Map<String, Object>> getServerToSegmentsMap(
+      @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName,
+      @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String 
tableTypeStr) {
+    List<String> tableNamesWithType =
+        getExistingTableNamesWithType(tableName, 
Constants.validateTableType(tableTypeStr));
+    List<Map<String, Object>> resultList = new 
ArrayList<>(tableNamesWithType.size());
+    for (String tableNameWithType : tableNamesWithType) {
+      Map<String, Object> resultForTable = new LinkedHashMap<>();
+      resultForTable.put("tableName", tableNameWithType);
+      resultForTable.put("serverToSegmentsMap", 
_pinotHelixResourceManager.getServerToSegmentsMap(tableNameWithType));
+      resultList.add(resultForTable);
     }
-    return toggleStateInternal(tableName, state, tableType, null, 
_pinotHelixResourceManager);
+    return resultList;
   }
 
+  @Deprecated
   @GET
-  @Path("tables/{tableName}/segments/{segmentName}")
+  @Path("tables/{tableName}/segments")
   @Produces(MediaType.APPLICATION_JSON)
-  @ApiOperation(value = "Lists metadata or toggles state of a segment", notes 
= "Toggles segment state if 'state' is specified in query param, otherwise 
lists segment metadata")
-  public String toggleStateOrListMetadataForOneSegment(
+  @ApiOperation(value = "Get a map from server to segments hosted by the 
server (deprecated, use 'GET /segments/{tableName}/servers' instead)", notes = 
"Get a map from server to segments hosted by the server (deprecated, use 'GET 
/segments/{tableName}/servers' instead)")
+  public List<Map<String, String>> getServerToSegmentsMapDeprecated1(
       @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName,
-      @ApiParam(value = "Name of segment", required = true) 
@PathParam("segmentName") @Encoded String segmentName,
-      @ApiParam(value = "enable|disable|drop", required = false) 
@QueryParam("state") String stateStr,
-      @ApiParam(value = "realtime|offline", required = false) 
@QueryParam("type") String tableTypeStr) {
-    segmentName = URIUtils.decode(segmentName);
-    // segmentName will never be null,otherwise we would reach the method 
toggleStateOrListMetadataForAllSegments()
-    CommonConstants.Helix.TableType tableType = 
Constants.validateTableType(tableTypeStr);
-    StateType stateType = Constants.validateState(stateStr);
-    if (stateStr == null) {
-      // This is a list metadata operation
-      return listSegmentMetadataInternal(tableName, segmentName, tableType);
-    } else {
-      // We need to toggle state
-      if (tableType == null) {
-        List<String> realtimeSegments =
-            
_pinotHelixResourceManager.getSegmentsFor(TableNameBuilder.REALTIME.tableNameWithType(tableName));
-        List<String> offlineSegments =
-            
_pinotHelixResourceManager.getSegmentsFor(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
-        if (realtimeSegments.contains(segmentName)) {
-          tableType = CommonConstants.Helix.TableType.REALTIME;
-        } else if (offlineSegments.contains(segmentName)) {
-          tableType = CommonConstants.Helix.TableType.OFFLINE;
-        }
-      }
-      return toggleStateInternal(tableName, stateType, tableType, segmentName, 
_pinotHelixResourceManager);
+      @ApiParam(value = "MUST be null") @QueryParam("state") String stateStr,
+      @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String 
tableTypeStr)
+      throws JsonProcessingException {
+    if (stateStr != null) {
+      throw new WebApplicationException("Cannot toggle segment state", 
Status.FORBIDDEN);
+    }
+
+    List<String> tableNamesWithType =
+        getExistingTableNamesWithType(tableName, 
Constants.validateTableType(tableTypeStr));
+    List<Map<String, String>> resultList = new 
ArrayList<>(tableNamesWithType.size());
+    for (String tableNameWithType : tableNamesWithType) {
+      // NOTE: DO NOT change the format for backward-compatibility
+      Map<String, String> resultForTable = new LinkedHashMap<>();
+      resultForTable.put("tableName", tableNameWithType);
+      resultForTable.put("segments",
+          
JsonUtils.objectToString(_pinotHelixResourceManager.getServerToSegmentsMap(tableNameWithType)));
+      resultList.add(resultForTable);
     }
+    return resultList;
   }
 
+  @Deprecated
   @GET
   @Path("tables/{tableName}/segments/metadata")
   @Produces(MediaType.APPLICATION_JSON)
-  @ApiOperation(value = "Lists metadata all segments of table", notes = "Lists 
segment metadata")
-  // TODO: 1. stateStr is not used
-  // TODO: 2. this method returns the map from server to segments instead of 
segment metadata
-  public String listMetadataForAllSegments(
+  @ApiOperation(value = "Get a map from server to segments hosted by the 
server (deprecated, use 'GET /segments/{tableName}/servers' instead)", notes = 
"Get a map from server to segments hosted by the server (deprecated, use 'GET 
/segments/{tableName}/servers' instead)")
+  public List<Map<String, String>> getServerToSegmentsMapDeprecated2(
       @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName,
-      @ApiParam(value = "enable|disable|drop", required = false) 
@QueryParam("state") String stateStr,
-      @ApiParam(value = "realtime|offline", required = false) 
@QueryParam("type") String tableTypeStr) {
-    CommonConstants.Helix.TableType tableType = 
Constants.validateTableType(tableTypeStr);
-    return getInstanceToSegmentsMap(tableName, tableType);
+      @ApiParam(value = "MUST be null") @QueryParam("state") String stateStr,
+      @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String 
tableTypeStr)
+      throws JsonProcessingException {
+    return getServerToSegmentsMapDeprecated1(tableName, stateStr, 
tableTypeStr);
   }
 
   @GET
-  @Path("tables/{tableName}/segments/{segmentName}/metadata")
+  @Path("segments/{tableName}/crc")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Get a map from segment to CRC of the segment (only 
apply to OFFLINE table)", notes = "Get a map from segment to CRC of the segment 
(only apply to OFFLINE table)")
+  public Map<String, String> getSegmentToCrcMap(
+      @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName) {
+    String offlineTableName = getExistingTableNamesWithType(tableName, 
TableType.OFFLINE).get(0);
+    return _pinotHelixResourceManager.getSegmentsCrcForTable(offlineTableName);
+  }
+
+  @Deprecated
+  @GET
+  @Path("tables/{tableName}/segments/crc")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Get a map from segment to CRC of the segment 
(deprecated, use 'GET /segments/{tableName}/crc' instead)", notes = "Get a map 
from segment to CRC of the segment (deprecated, use 'GET 
/segments/{tableName}/crc' instead)")
+  public Map<String, String> getSegmentToCrcMapDeprecated(
+      @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName) {
+    return getSegmentToCrcMap(tableName);
+  }
+
+  @GET
+  @Path("segments/{tableName}/{segmentName}/metadata")
   @Produces(MediaType.APPLICATION_JSON)
-  @ApiOperation(value = "Lists one segment's metadata", notes = "Lists one 
segment's metadata")
-  public String listMetadataForOneSegment(
+  @ApiOperation(value = "Get the metadata for a segment", notes = "Get the 
metadata for a segment")
+  public List<Map<String, Object>> getSegmentMetadata(
       @ApiParam(value = "Name of the table", required = true) 
@PathParam("tableName") String tableName,
-      @ApiParam(value = "Name of segment", required = true) 
@PathParam("segmentName") @Encoded String segmentName,
-      @ApiParam(value = "realtime|offline", required = false) 
@QueryParam("type") String tableTypeStr) {
+      @ApiParam(value = "Name of the segment", required = true) 
@PathParam("segmentName") @Encoded String segmentName,
+      @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String 
tableTypeStr) {
     segmentName = URIUtils.decode(segmentName);
-    CommonConstants.Helix.TableType tableType = 
Constants.validateTableType(tableTypeStr);
-    return listSegmentMetadataInternal(tableName, segmentName, tableType);
+    TableType tableType = Constants.validateTableType(tableTypeStr);
+    List<String> tableNamesWithType = getExistingTableNamesWithType(tableName, 
tableType);
+    List<Map<String, Object>> resultList = new 
ArrayList<>(tableNamesWithType.size());
+    for (String tableNameWithType : tableNamesWithType) {
+      Map<String, String> segmentMetadata = 
getSegmentMetadata(tableNameWithType, segmentName);
+      if (segmentMetadata != null) {
+        Map<String, Object> resultForTable = new LinkedHashMap<>();
+        resultForTable.put("tableName", tableNameWithType);
+        resultForTable.put("segmentMetadata", segmentMetadata);
+        resultList.add(resultForTable);
+      }
+    }
+    if (resultList.isEmpty()) {
+      String errorMessage =
+          "Failed to find segment: " + segmentName + " in table: " + tableName 
+ (tableType != null ? (" of type: "
+              + tableType) : "");
+      throw new ControllerApplicationException(LOGGER, errorMessage, 
Status.NOT_FOUND);
+    }
+    return resultList;
   }
 
-  private String listSegmentMetadataInternal(@Nonnull String tableName, 
@Nonnull String segmentName,
-      @Nullable CommonConstants.Helix.TableType tableType) {
-    List<String> tableNamesWithType = getExistingTableNamesWithType(tableName, 
tableType);
+  @Nullable
+  private Map<String, String> getSegmentMetadata(String tableNameWithType, 
String segmentName) {
+    ZkHelixPropertyStore<ZNRecord> propertyStore = 
_pinotHelixResourceManager.getPropertyStore();
+    if (TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableNameWithType)) {
+      OfflineSegmentZKMetadata offlineSegmentZKMetadata =
+          ZKMetadataProvider.getOfflineSegmentZKMetadata(propertyStore, 
tableNameWithType, segmentName);
+      return offlineSegmentZKMetadata != null ? 
offlineSegmentZKMetadata.toMap() : null;
+    } else {
+      RealtimeSegmentZKMetadata realtimeSegmentZKMetadata =
+          ZKMetadataProvider.getRealtimeSegmentZKMetadata(propertyStore, 
tableNameWithType, segmentName);
+      return realtimeSegmentZKMetadata != null ? 
realtimeSegmentZKMetadata.toMap() : null;
+    }
+  }
 
-    ArrayNode result = JsonUtils.newArrayNode();
+  @Deprecated
+  @GET
+  @Path("tables/{tableName}/segments/{segmentName}/metadata")
 
 Review comment:
   Incompatible with existing API. Currently, this API returns a String. Please 
keep the api the same.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

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

Reply via email to