[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

Jackie-Jiang commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248312080


##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java:
##########
@@ -163,6 +163,12 @@ public String forBrokerTablesGet(String state) {
     return StringUtil.join("/", _baseUrl, "brokers", "tables", "?state=" + state);
   }
 
+  public String forTenantInstancesToggle(String tenant, String tenantType, String state) {
+    StringBuilder url = new StringBuilder(StringUtil.join("/", _baseUrl, "tenants", tenant));
+    url.append("?type=").append(tenantType);
+    url.append("&state=").append(state);
+    return String.valueOf(url);
+  }

Review Comment:
   (nit) empty line below



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -203,19 +204,36 @@ public TenantsList getAllTenants(
   @GET
   @Path("/tenants/{tenantName}")
   @Produces(MediaType.APPLICATION_JSON)
-  @ApiOperation(value = "List instance for a tenant, or enable/disable/drop a tenant")
+  @ApiOperation(value = "List instance for a tenant")
   @ApiResponses(value = {
       @ApiResponse(code = 200, message = "Success"),
       @ApiResponse(code = 500, message = "Error reading tenants list")
   })
-  public String listInstanceOrToggleTenantState(
+  public String listInstance(
+      @ApiParam(value = "Tenant name", required = true) @PathParam("tenantName") String tenantName,
+      @ApiParam(value = "Tenant type (server|broker)") @QueryParam("type") String tenantType,
+      @ApiParam(value = "Table type (offline|realtime)") @QueryParam("tableType") String tableType) {
+    return listInstancesForTenant(tenantName, tenantType, tableType);
+  }
+
+  @POST
+  @Path("/tenants/{tenantName}")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "enable/disable/drop a tenant")
+  @ApiResponses(value = {
+      @ApiResponse(code = 200, message = "Success"),
+      @ApiResponse(code = 500, message = "Error applying state to tenant")
+  })
+  public SuccessResponse toggleTenantState(

Review Comment:
   Seems we also have `changeTenantState()` for the same purpose. Can you check if they are doing the exact same thing? If so, we should remove the duplicate logic, and mark `changeTenantState()` as deprecated



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -284,19 +302,24 @@ private String toggleTenantState(String tenantName, String stateStr, @Nullable S
       _pinotHelixResourceManager.deleteBrokerTenantFor(tenantName);
       _pinotHelixResourceManager.deleteOfflineServerTenantFor(tenantName);
       _pinotHelixResourceManager.deleteRealtimeServerTenantFor(tenantName);
-      return new SuccessResponse("Dropped tenant " + tenantName + " successfully.").toString();
+      return new SuccessResponse("Dropped tenant " + tenantName + " successfully.");
     }
 
-    boolean enable = StateType.ENABLE.name().equalsIgnoreCase(stateStr) ? true : false;
-    for (String instance : allInstances) {
-      if (enable) {
+    if (StateType.ENABLE.name().equalsIgnoreCase(stateStr)) {
+      for (String instance : allInstances) {
         instanceResult.put(instance, JsonUtils.objectToJsonNode(_pinotHelixResourceManager.enableInstance(instance)));
-      } else {
+      }
+      return new SuccessResponse("Enabled tenant " + tenantName + " successfully.");
+    }
+
+    if (StateType.DISABLE.name().equalsIgnoreCase(stateStr)) {
+      for (String instance : allInstances) {
         instanceResult.put(instance, JsonUtils.objectToJsonNode(_pinotHelixResourceManager.disableInstance(instance)));
       }
+      return new SuccessResponse("Disabled tenant " + tenantName + " successfully.");
     }
 
-    return null;
+    return new SuccessResponse("No-Op done on tenant " + tenantName);

Review Comment:
   Will we ever reach this branch?



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -203,19 +204,36 @@ public TenantsList getAllTenants(
   @GET
   @Path("/tenants/{tenantName}")
   @Produces(MediaType.APPLICATION_JSON)
-  @ApiOperation(value = "List instance for a tenant, or enable/disable/drop a tenant")
+  @ApiOperation(value = "List instance for a tenant")
   @ApiResponses(value = {
       @ApiResponse(code = 200, message = "Success"),
       @ApiResponse(code = 500, message = "Error reading tenants list")
   })
-  public String listInstanceOrToggleTenantState(
+  public String listInstance(
+      @ApiParam(value = "Tenant name", required = true) @PathParam("tenantName") String tenantName,
+      @ApiParam(value = "Tenant type (server|broker)") @QueryParam("type") String tenantType,
+      @ApiParam(value = "Table type (offline|realtime)") @QueryParam("tableType") String tableType) {
+    return listInstancesForTenant(tenantName, tenantType, tableType);
+  }
+
+  @POST
+  @Path("/tenants/{tenantName}")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "enable/disable/drop a tenant")

Review Comment:
   Suggest only allowing enable and disable. Drop should be achieved by a DELETE request



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org