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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
