You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/07/21 23:00:34 UTC
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5724: Enable/disable query quotas per broker
mcvsubbu commented on a change in pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#discussion_r458431852
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java
##########
@@ -359,6 +372,27 @@ public void processQueryQuotaChange(ExternalView currentBrokerResource) {
numRebuilt, _rateLimiterMap.size());
}
+ /**
+ * Process query quota state change when instance config gets changed
+ */
+ public void processQueryQuotaInstanceConfigChange() {
+ getQueryQuotaEnabledFlagFromInstanceConfig();
+ }
+
+ private void getQueryQuotaEnabledFlagFromInstanceConfig() {
+ Map<String, String> instanceConfigsMap =
+ HelixHelper.getInstanceConfigsMapFor(_instanceId, _helixManager.getClusterName(),
+ _helixManager.getClusterManagmentTool());
+ String queryQuotaEnabled = instanceConfigsMap.getOrDefault(CommonConstants.Helix.QUERY_QUOTA_STATE_ENABLED, "true");
+ _enabled = Boolean.parseBoolean(queryQuotaEnabled);
+ LOGGER.info("Set query quota state to: {} for {} tables in the current broker.", _enabled ? "ENABLE" : "DISABLE",
Review comment:
```suggestion
LOGGER.info("Set query rate limiting to: {} for all tables in this broker.", _enabled ? "ENABLED" : "DISABLED",
```
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -30,6 +30,7 @@
public static class Helix {
public static final String IS_SHUTDOWN_IN_PROGRESS = "shutdownInProgress";
public static final String QUERIES_DISABLED = "queriesDisabled";
+ public static final String QUERY_QUOTA_STATE_ENABLED = "queryQuotaStateEnabled";
Review comment:
```suggestion
public static final String QUERY_RATE_LIMIT_DISABLED = "queryRateLimitDisabled";
```
Since this is not the normal state, IMO it is better to highlight if this is disabled rather than enabled.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -129,6 +133,40 @@
}
}
+
+ @POST
+ @Path("/brokers/instances/{instanceName}/qps")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.TEXT_PLAIN)
+ @ApiOperation(value = "Enable/disable the qps quotas of an broker instance", notes = "Enable/disable the qps quotas of an broker instance")
Review comment:
```suggestion
@ApiOperation(value = "Enable/disable the query rate limiting for a broker instance", notes = "Enable/disable the query rate limiting for a broker instance")
```
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java
##########
@@ -359,6 +372,27 @@ public void processQueryQuotaChange(ExternalView currentBrokerResource) {
numRebuilt, _rateLimiterMap.size());
}
+ /**
+ * Process query quota state change when instance config gets changed
+ */
+ public void processQueryQuotaInstanceConfigChange() {
+ getQueryQuotaEnabledFlagFromInstanceConfig();
+ }
+
+ private void getQueryQuotaEnabledFlagFromInstanceConfig() {
+ Map<String, String> instanceConfigsMap =
+ HelixHelper.getInstanceConfigsMapFor(_instanceId, _helixManager.getClusterName(),
+ _helixManager.getClusterManagmentTool());
+ String queryQuotaEnabled = instanceConfigsMap.getOrDefault(CommonConstants.Helix.QUERY_QUOTA_STATE_ENABLED, "true");
+ _enabled = Boolean.parseBoolean(queryQuotaEnabled);
+ LOGGER.info("Set query quota state to: {} for {} tables in the current broker.", _enabled ? "ENABLE" : "DISABLE",
+ _rateLimiterMap.size());
Review comment:
```suggestion
);
```
Even if new tables are added, query rate limiting check will not be done. Maybe you can add in the same message that there are currently N tables on this broker, but the log should clearly indicate that it is disabled for all tables
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -129,6 +133,40 @@
}
}
+
+ @POST
+ @Path("/brokers/instances/{instanceName}/qps")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.TEXT_PLAIN)
+ @ApiOperation(value = "Enable/disable the qps quotas of an broker instance", notes = "Enable/disable the qps quotas of an broker instance")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 404, message = "Instance not found"), @ApiResponse(code = 409, message = "Instance cannot be dropped"), @ApiResponse(code = 500, message = "Internal error")})
+ public SuccessResponse toggleBrokerQpsState(
+ @ApiParam(value = "Broker instance name", required = true, example = "Broker_my.broker.com_30000") @PathParam("instanceName") String brokerInstanceName,
+ @ApiParam(value = "ENABLE|DISABLE", allowableValues = "ENABLE, DISABLE", required = true) @QueryParam("state") String state) {
+ List<String> liveInstances = _pinotHelixResourceManager.getOnlineInstanceList();
+ if (brokerInstanceName == null || !brokerInstanceName.startsWith("Broker_")) {
+ LOGGER.info("{} isn't a valid broker instance name", brokerInstanceName);
+ throw new ControllerApplicationException(LOGGER,
+ String.format("'%s' is not a valid broker instance name.", brokerInstanceName), Response.Status.BAD_REQUEST);
+ }
+ if (!liveInstances.contains(brokerInstanceName)) {
+ LOGGER.info("Broker instance: {} not found from live instances", brokerInstanceName);
+ throw new ControllerApplicationException(LOGGER, String.format("Instance '%s' not found.", brokerInstanceName),
+ Response.Status.NOT_FOUND);
+ }
+ validateQueryQuotaStateChange(state);
+ _pinotHelixResourceManager.toggleQueryQuotaStateForBroker(brokerInstanceName, state);
+ String msg = String.format("Toggled query quota state to: %s for broker: %s", state, brokerInstanceName);
Review comment:
```suggestion
String msg = String.format("Set query rate limiting to: %s for all tables in broker: %s", state, brokerInstanceName);
```
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java
##########
@@ -359,6 +372,27 @@ public void processQueryQuotaChange(ExternalView currentBrokerResource) {
numRebuilt, _rateLimiterMap.size());
}
+ /**
+ * Process query quota state change when instance config gets changed
+ */
+ public void processQueryQuotaInstanceConfigChange() {
+ getQueryQuotaEnabledFlagFromInstanceConfig();
+ }
+
+ private void getQueryQuotaEnabledFlagFromInstanceConfig() {
+ Map<String, String> instanceConfigsMap =
+ HelixHelper.getInstanceConfigsMapFor(_instanceId, _helixManager.getClusterName(),
+ _helixManager.getClusterManagmentTool());
+ String queryQuotaEnabled = instanceConfigsMap.getOrDefault(CommonConstants.Helix.QUERY_QUOTA_STATE_ENABLED, "true");
Review comment:
```suggestion
String queryQuotaEnabled = instanceConfigsMap.getOrDefault(CommonConstants.Helix.QUERY_QUOTA_STATE_ENABLED, "false");
```
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -129,6 +133,40 @@
}
}
+
+ @POST
+ @Path("/brokers/instances/{instanceName}/qps")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.TEXT_PLAIN)
+ @ApiOperation(value = "Enable/disable the qps quotas of an broker instance", notes = "Enable/disable the qps quotas of an broker instance")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 404, message = "Instance not found"), @ApiResponse(code = 409, message = "Instance cannot be dropped"), @ApiResponse(code = 500, message = "Internal error")})
+ public SuccessResponse toggleBrokerQpsState(
Review comment:
```suggestion
public SuccessResponse toggleQueryRateLimiting(
```
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -129,6 +133,40 @@
}
}
+
+ @POST
+ @Path("/brokers/instances/{instanceName}/qps")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.TEXT_PLAIN)
+ @ApiOperation(value = "Enable/disable the qps quotas of an broker instance", notes = "Enable/disable the qps quotas of an broker instance")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 404, message = "Instance not found"), @ApiResponse(code = 409, message = "Instance cannot be dropped"), @ApiResponse(code = 500, message = "Internal error")})
+ public SuccessResponse toggleBrokerQpsState(
+ @ApiParam(value = "Broker instance name", required = true, example = "Broker_my.broker.com_30000") @PathParam("instanceName") String brokerInstanceName,
+ @ApiParam(value = "ENABLE|DISABLE", allowableValues = "ENABLE, DISABLE", required = true) @QueryParam("state") String state) {
+ List<String> liveInstances = _pinotHelixResourceManager.getOnlineInstanceList();
Review comment:
Can you move this line down after the parameter checking? thanks
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotBrokerRestletResource.java
##########
@@ -129,6 +133,40 @@
}
}
+
+ @POST
+ @Path("/brokers/instances/{instanceName}/qps")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.TEXT_PLAIN)
+ @ApiOperation(value = "Enable/disable the qps quotas of an broker instance", notes = "Enable/disable the qps quotas of an broker instance")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 404, message = "Instance not found"), @ApiResponse(code = 409, message = "Instance cannot be dropped"), @ApiResponse(code = 500, message = "Internal error")})
Review comment:
What does `409` mean in this case?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org