You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2023/06/28 15:35:03 UTC
[GitHub] [pinot] tibrewalpratik17 opened a new pull request, #10993: Update getTenantInstances call for controller and separate POST operations on it
tibrewalpratik17 opened a new pull request, #10993:
URL: https://github.com/apache/pinot/pull/10993
labels:
1. `bugfix`
2. `backward-incompatible`
We found an issue with `listInstanceOrToggleTenantState` endpoint today in our production.
- We do a POST operation in a API which is marked as GET. This leads to confusion and treats this API as mainly a NOOP.
- We don't check if the state passed is a valid state in the list of enums - ENABLE, DISABLE, DROP.
- If the state is not valid, we end-up disabling all the instances.
This led to ingestion and query failures in our production cluster.
This PR separates the POST operation in a separate POST API. Adds a check for matching the state passed in the API parameter. Disables instances only when `DISABLE` is passed as the State.
--
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
[GitHub] [pinot] tibrewalpratik17 commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248657080
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -260,7 +276,7 @@ private String getTablesServedFromTenant(String tenantName) {
return resourceGetRet.toString();
}
- private String toggleTenantState(String tenantName, String stateStr, @Nullable String tenantType) {
+ private SuccessResponse toggleTenantState(String tenantName, String stateStr, @Nullable String tenantType) {
Review Comment:
Now we are only using 2 states here - enable / disable. So toggle still makes sense. DROP is taken care of deleteTenant API which was already there.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "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
[GitHub] [pinot] chenboat commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248389459
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -276,27 +292,17 @@ private String toggleTenantState(String tenantName, String stateStr, @Nullable S
Set<String> allInstances = new HashSet<String>(serverInstances);
allInstances.addAll(brokerInstances);
- if (StateType.DROP.name().equalsIgnoreCase(stateStr)) {
- if (!allInstances.isEmpty()) {
- throw new ControllerApplicationException(LOGGER,
- "Error: Tenant " + tenantName + " has live instances, cannot be dropped.", Response.Status.BAD_REQUEST);
+ if (StateType.DISABLE.name().equalsIgnoreCase(stateStr)) {
+ for (String instance : allInstances) {
+ instanceResult.put(instance, JsonUtils.objectToJsonNode(_pinotHelixResourceManager.disableInstance(instance)));
}
- _pinotHelixResourceManager.deleteBrokerTenantFor(tenantName);
- _pinotHelixResourceManager.deleteOfflineServerTenantFor(tenantName);
- _pinotHelixResourceManager.deleteRealtimeServerTenantFor(tenantName);
- return new SuccessResponse("Dropped tenant " + tenantName + " successfully.").toString();
}
-
- 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 {
- instanceResult.put(instance, JsonUtils.objectToJsonNode(_pinotHelixResourceManager.disableInstance(instance)));
}
}
-
- return null;
+ return new SuccessResponse("Changed state of tenant " + tenantName + " to " + stateStr + " successfully.");
Review Comment:
There are three states: can we add the from state to the response?
--
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
[GitHub] [pinot] tibrewalpratik17 commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248335615
##########
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:
I see there is already a delete tenant API so removing it from here.
--
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
[GitHub] [pinot] chenboat commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248392840
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -260,7 +276,7 @@ private String getTablesServedFromTenant(String tenantName) {
return resourceGetRet.toString();
}
- private String toggleTenantState(String tenantName, String stateStr, @Nullable String tenantType) {
+ private SuccessResponse toggleTenantState(String tenantName, String stateStr, @Nullable String tenantType) {
Review Comment:
toggle means switch between 2 states. But the StateType enum has 3 states. We should use changeTenantState to reduce the confusion -- unless you can explain in javadoc why some state is not used.
--
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
[GitHub] [pinot] tibrewalpratik17 commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248335682
##########
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:
Yes looks like the logic is same. Marked the other one 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:
Removed.
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10993:
URL: https://github.com/apache/pinot/pull/10993#issuecomment-1611746937
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10993?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10993](https://app.codecov.io/gh/apache/pinot/pull/10993?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a63d2af) into [master](https://app.codecov.io/gh/apache/pinot/commit/45a32eaaad594955754732ec18ee78abeb8e96b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (45a32ea) will **decrease** coverage by `0.12%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10993 +/- ##
==========================================
- Coverage 0.11% 0.00% -0.12%
==========================================
Files 2192 2176 -16
Lines 118085 117655 -430
Branches 17885 17831 -54
==========================================
- Hits 137 0 -137
+ Misses 117928 117655 -273
+ Partials 20 0 -20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `0.00% <0.00%> (ø)` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `0.00% <0.00%> (ø)` | |
| unittests1temurin11 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10993?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ller/api/resources/PinotTenantRestletResource.java](https://app.codecov.io/gh/apache/pinot/pull/10993?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGVuYW50UmVzdGxldFJlc291cmNlLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [18 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10993/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10993:
URL: https://github.com/apache/pinot/pull/10993
--
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
[GitHub] [pinot] tibrewalpratik17 commented on pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #10993:
URL: https://github.com/apache/pinot/pull/10993#issuecomment-1612237184
cc @chenboat
--
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
[GitHub] [pinot] tibrewalpratik17 commented on a diff in pull request #10993: Update getTenantInstances call for controller and separate POST operations on it
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10993:
URL: https://github.com/apache/pinot/pull/10993#discussion_r1248657503
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -276,27 +292,17 @@ private String toggleTenantState(String tenantName, String stateStr, @Nullable S
Set<String> allInstances = new HashSet<String>(serverInstances);
allInstances.addAll(brokerInstances);
- if (StateType.DROP.name().equalsIgnoreCase(stateStr)) {
- if (!allInstances.isEmpty()) {
- throw new ControllerApplicationException(LOGGER,
- "Error: Tenant " + tenantName + " has live instances, cannot be dropped.", Response.Status.BAD_REQUEST);
+ if (StateType.DISABLE.name().equalsIgnoreCase(stateStr)) {
+ for (String instance : allInstances) {
+ instanceResult.put(instance, JsonUtils.objectToJsonNode(_pinotHelixResourceManager.disableInstance(instance)));
}
- _pinotHelixResourceManager.deleteBrokerTenantFor(tenantName);
- _pinotHelixResourceManager.deleteOfflineServerTenantFor(tenantName);
- _pinotHelixResourceManager.deleteRealtimeServerTenantFor(tenantName);
- return new SuccessResponse("Dropped tenant " + tenantName + " successfully.").toString();
}
-
- 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 {
- instanceResult.put(instance, JsonUtils.objectToJsonNode(_pinotHelixResourceManager.disableInstance(instance)));
}
}
-
- return null;
+ return new SuccessResponse("Changed state of tenant " + tenantName + " to " + stateStr + " successfully.");
Review Comment:
We are using 2 states in this API now just enable/disable.
--
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