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 18:26:44 UTC
[GitHub] [incubator-pinot] jackjlli opened a new pull request #5724: Enable/disable query quotas per broker
jackjlli opened a new pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724
## Description
This PR adds logic to enable/disable query quotas per Pinot broker.
This is helpful to test out the capacity of a given broker without affecting other brokers in production. Higher load can be routed to this dedicated host to find out the capacity, which can give us better insights on the current provisioning of the cluster. LinkedIn currently has one service that has been testing in this way.
The query quotas are all enabled/disabled together within a broker. Once the broker gets restarted, query quotas would all be re-enabled again whatever the previous states were before.
## Tests done:
Started all Pinot components.
Broker log:
```
2020/07/21 11:12:24.296 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] Initializing rate limiter for table internalTesting_OFFLINE
2020/07/21 11:12:24.296 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] The number of online brokers for table internalTesting_OFFLINE is 1
2020/07/21 11:12:24.300 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] Rate limiter for table: internalTesting_OFFLINE has been initialized. Overall rate: 0.01. P
er-broker rate: 0.01. Number of online broker instances: 1. Table config stat version: 0
...
2020/07/21 11:13:22.291 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-0] requestId=1,table=internalTesting_OFFLINE,timeMs=594,docs=10/970,entries=0/240,segments(queried/processed/matched/consuming/unavailable):1/1/1/0/0,consumingFreshnessTimeMs=0,servers=1/1,groupLimitReached=false,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs);172.18.180.84_O=95,144,2756,4,query=select * from internalTesting limit 10
2020/07/21 11:13:32.779 INFO [HelixExternalViewBasedQueryQuotaManager] [jersey-server-managed-async-executor-1] Quota is exceeded for table: internalTesting_OFFLINE. Per-broker rate: 0.01. Current qps: 1
2020/07/21 11:13:32.782 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-1] Request 2 exceeds query quota for table:internalTesting, query:select * from internalTesting limit 50
2020/07/21 11:13:41.725 INFO [HelixExternalViewBasedQueryQuotaManager] [jersey-server-managed-async-executor-2] Quota is exceeded for table: internalTesting_OFFLINE. Per-broker rate: 0.01. Current qps: 1
2020/07/21 11:13:41.725 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-2] Request 3 exceeds query quota for table:internalTesting, query:select * from internalTesting limit 30
```
Make the API call to disable query quotas:
```
2020/07/21 11:14:01.137 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] Toggle query quota state to: DISABLE
2020/07/21 11:14:01.137 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] Finished toggling query quota state to: DISABLE for 1 tables in the current broker.
2020/07/21 11:14:19.332 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-3] requestId=4,table=internalTesting_OFFLINE,timeMs=13,docs=20/970,entries=0/480,segments(queried/processed/matched/consuming/unavailable):1/1/1/0/0,consumingFreshnessTimeMs=0,servers=1/1,groupLimitReached=false,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs);172.18.180.84_O=0,8,4515,0,query=select * from internalTesting limit 20
2020/07/21 11:14:22.999 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-4] requestId=5,table=internalTesting_OFFLINE,timeMs=10,docs=40/970,entries=0/960,segments(queried/processed/matched/consuming/unavailable):1/1/1/0/0,consumingFreshnessTimeMs=0,servers=1/1,groupLimitReached=false,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs);172.18.180.84_O=0,6,8035,0,query=select * from internalTesting limit 40
2020/07/21 11:14:26.120 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-5] requestId=6,table=internalTesting_OFFLINE,timeMs=18,docs=60/970,entries=0/1440,segments(queried/processed/matched/consuming/unavailable):1/1/1/0/0,consumingFreshnessTimeMs=0,servers=1/1,groupLimitReached=false,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs);172.18.180.84_O=0,11,11556,0,query=select * from internalTesting limit 60
```
Re-enable query quotas:
```
2020/07/21 11:14:36.406 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] Toggle query quota state to: ENABLE
2020/07/21 11:14:36.406 INFO [HelixExternalViewBasedQueryQuotaManager] [HelixTaskExecutor-message_handle_thread] Finished toggling query quota state to: ENABLE for 1 tables in the current broker.
2020/07/21 11:14:50.054 INFO [HelixExternalViewBasedQueryQuotaManager] [jersey-server-managed-async-executor-6] Quota is exceeded for table: internalTesting_OFFLINE. Per-broker rate: 0.01. Current qps: 1
2020/07/21 11:14:50.054 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-6] Request 7 exceeds query quota for table:internalTesting, query:select * from internalTesting limit 70
2020/07/21 11:14:53.956 INFO [HelixExternalViewBasedQueryQuotaManager] [jersey-server-managed-async-executor-7] Quota is exceeded for table: internalTesting_OFFLINE. Per-broker rate: 0.01. Current qps: 1
2020/07/21 11:14:53.956 INFO [BaseBrokerRequestHandler] [jersey-server-managed-async-executor-7] Request 8 exceeds query quota for table:internalTesting, query:select * from internalTesting limit 80
```
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#issuecomment-662041528
If I understand this right, we are disabling quota check on brokers dynamically. Assuming such dynamic enable/disable is a requirement, why not provide a (rest) API to do it? Why use helix messages?
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#discussion_r459088078
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java
##########
@@ -352,13 +369,42 @@ public void processQueryQuotaChange(ExternalView currentBrokerResource) {
numRebuilt++;
}
}
+ if (isQueryRateLimitDisabled()) {
+ LOGGER.info("Query rate limiting is currently disabled for this broker. So it won't take effect immediately.");
+ }
_lastKnownBrokerResourceVersion.set(currentVersionNumber);
long endTime = System.currentTimeMillis();
LOGGER
.info("Processed query quota change in {}ms, {} out of {} query quota configs rebuilt.", (endTime - startTime),
numRebuilt, _rateLimiterMap.size());
}
+ /**
+ * Process query quota state change when instance config gets changed
+ */
+ public void processQueryQuotaInstanceConfigChange() {
Review comment:
suggest renaming these two methods as per new change in config names
##########
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 query rate limiting for a broker instance", notes = "Enable/disable the query rate limiting for a broker instance")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 404, message = "Instance not found"), @ApiResponse(code = 500, message = "Internal error")})
+ public SuccessResponse toggleQueryRateLimiting(
+ @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) {
+ 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);
Review comment:
nit: Create an error string, and then use it in log as well as exeption.
Same in other places as well, will not repeat.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli edited a comment on pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli edited a comment on pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#issuecomment-662134507
> It will also be confusing if the broker restarts after receiving the message for any reason. It will stop checking for quotas, making debugging harder.
> Why not use instance config to set/unset a flag to disable qps quotas?
Updated the PR so that the state gets persisted to broker's instance config.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#discussion_r459094346
##########
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 query rate limiting for a broker instance", notes = "Enable/disable the query rate limiting for a broker instance")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 400, message = "Bad Request"), @ApiResponse(code = 404, message = "Instance not found"), @ApiResponse(code = 500, message = "Internal error")})
+ public SuccessResponse toggleQueryRateLimiting(
+ @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) {
+ 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);
Review comment:
Removed the duplicate logging.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli commented on pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#issuecomment-662066684
> If I understand this right, we are disabling quota check on brokers dynamically. Assuming such dynamic enable/disable is a requirement, why not provide a (rest) API to do it? Why use helix messages?
The reason why I put the API to pinot controller and send a Helix message to broker is that we've already been using this way to update query quotas to brokers (when table config is updated).
Plus, all the logic of cluster management should all be in one place, which is in Pinot controller.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#discussion_r458452344
##########
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:
Done.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli commented on pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#issuecomment-662134507
> It will also be confusing if the broker restarts after receiving the message for any reason. It will stop checking for quotas, making debugging harder.
> Why not use instance config to set/unset a flag to disable qps quotas?
Updated the PR so that the state gets persisted to broker's instance config
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#issuecomment-662113270
It will also be confusing if the broker restarts after receiving the message for any reason. It will stop checking for quotas, making debugging harder.
Why not use instance config to set/unset a flag to disable qps quotas?
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli merged pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli merged pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#discussion_r458441365
##########
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:
I'd insist keeping the size of the map. That'll give us a good insight on how many tables are affected by this state change.
And sure, I can add the state message in the message.
----------------------------------------------------------------
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
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#discussion_r458441792
##########
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:
Good catch! 👍 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.
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] [incubator-pinot] jackjlli commented on pull request #5724: Enable/disable query quotas per broker
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #5724:
URL: https://github.com/apache/incubator-pinot/pull/5724#issuecomment-662164187
> Mostly minor comments.
> Two things:
>
> 1. Should we consider accepting a broker tag name in the API to toggle rate limiting for all brokers in the tag? (as a general functionality, not specific to LinkedIn use case)
> 2. Add a metric (guage) when the config is disabled, so that if it remains disabled for a long time, we can add an alert on it
For 1, I can't think of a scenario for that. But we can always add that later if necessary.
For 2, just added a broker gauge called `QUERY_RATE_LIMIT_DISABLED`.
----------------------------------------------------------------
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