xiangfu0 commented on code in PR #20235:
URL: https://github.com/apache/pinot/pull/20235#discussion_r4546215514
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -381,6 +384,100 @@ public InstancePartitions assignInstancesPartitionMap(
return instancePartitions;
}
+ @GET
+ @Path("/tenants/{tenantName}/quotas")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_TENANT)
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the quota defaults of a tenant",
+ notes = "Storage quota defaults apply to the tables on the server tenant
and query quota defaults to the tables "
+ + "on the broker tenant, unless a table sets the corresponding field
in its own quota config")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success", response =
QuotaConfig.class),
+ @ApiResponse(code = 404, message = "Quota defaults not found")
+ })
+ public QuotaConfig getTenantQuotaConfig(
+ @ApiParam(value = "Tenant name", required = true)
@PathParam("tenantName") String tenantName) {
+ QuotaConfig quotaConfig =
_pinotHelixResourceManager.getTenantQuotaConfig(tenantName);
+ if (quotaConfig == null) {
+ throw new ControllerApplicationException(LOGGER, "Quota defaults not
found for tenant: " + tenantName,
+ Response.Status.NOT_FOUND);
+ }
+ return quotaConfig;
+ }
+
+ @PUT
+ @Path("/tenants/{tenantName}/quotas")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.UPDATE_TENANT)
+ @Authenticate(AccessType.UPDATE)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Set the quota defaults of a tenant",
+ notes = "Storage quota defaults (storage, storageQuotaEnforcementMode)
apply to the tables on the server tenant "
+ + "and take effect on the next storage quota check. Query quota
defaults (maxQueriesPerSecond, "
+ + "queryQuotaEnforcementMode) apply to the tables on the broker
tenant and take effect when brokers process "
+ + "the refresh message. A table that sets a field in its own quota
config overrides the default. "
+ + "Enforcement modes are inherited too, so setting a limit without a
mode enforces it on every table that "
+ + "does not set its own mode; set the mode to SHADOW first to
observe breaches before enforcing")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 400, message = "Invalid quota config"),
+ @ApiResponse(code = 404, message = "Tenant not found"),
+ @ApiResponse(code = 500, message = "Error updating the quota defaults")
+ })
+ public SuccessResponse setTenantQuotaConfig(
+ @ApiParam(value = "Tenant name", required = true)
@PathParam("tenantName") String tenantName,
+ String quotaConfigStr) {
+ boolean isServerTenant =
_pinotHelixResourceManager.getAllServerTenantNames().contains(tenantName);
+ boolean isBrokerTenant =
_pinotHelixResourceManager.getAllBrokerTenantNames().contains(tenantName);
+ if (!isServerTenant && !isBrokerTenant) {
+ throw new ControllerApplicationException(LOGGER, "Tenant not found: " +
tenantName, Response.Status.NOT_FOUND);
+ }
+ QuotaConfig quotaConfig;
+ try {
+ quotaConfig = JsonUtils.stringToObject(quotaConfigStr,
QuotaConfig.class);
+ Preconditions.checkArgument(quotaConfig != null, "Quota config must be
provided");
+ QuotaConfigUtils.validateTenantQuotaConfig(quotaConfig, isServerTenant,
isBrokerTenant);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, "Invalid quota config:
" + e.getMessage(),
+ Response.Status.BAD_REQUEST, e);
+ }
+ try {
+ _pinotHelixResourceManager.setTenantQuotaConfig(tenantName, quotaConfig);
Review Comment:
This PUT replaces the whole `/CONFIGS/TENANT/<name>` record, which holds
both the storage (server-role) and QPS (broker-role) defaults. On a tenant that
is both, such as `DefaultTenant` in most clusters, `PUT
{"maxQueriesPerSecond":"100"}` silently drops an existing storage default and
loosens storage enforcement on every table. Could this merge per role,
replacing only the fields of the role being set? If not, the API notes should
at least say that a PUT replaces both roles.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QuotaConfigUtils.java:
##########
@@ -0,0 +1,124 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils.config;
+
+import com.google.common.base.Preconditions;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.config.table.QuotaConfig;
+import org.apache.pinot.spi.config.table.QuotaEnforcementMode;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/// Resolves the effective quota of a table from its own [QuotaConfig] and the
quota defaults of its tenants.
+///
+/// Each quota is inherited from the tenant whose role enforces it:
+/// - storage (and its enforcement mode) from the table's server tenant,
enforced by the controller
+/// - query QPS (and its enforcement mode) from the table's broker tenant,
enforced by the brokers
+///
+/// Resolution is per field: a field set in the table quota config wins,
otherwise the tenant default applies. Each
+/// enforcement mode is resolved independently of its limit, so a tenant can
e.g. put all its tables in SHADOW mode
+/// while tables still set their own limits, and a table can opt out of a
tenant limit by setting the corresponding
+/// enforcement mode to DISABLED.
+///
+/// Stateless and thread-safe.
+public class QuotaConfigUtils {
+ private QuotaConfigUtils() {
+ }
+
+ /// Returns the server tenant whose storage quota defaults apply to the
table, which is the server tenant configured
+ /// in the table's tenant config, or the default tenant when none is
configured. Tag overrides are not considered.
+ public static String getServerTenantName(TableConfig tableConfig) {
+ String serverTenant = tableConfig.getTenantConfig().getServer();
+ return serverTenant != null ? serverTenant :
TagNameUtils.DEFAULT_TENANT_NAME;
+ }
+
+ /// Returns the broker tenant whose query quota defaults apply to the table,
which is the broker tenant configured in
+ /// the table's tenant config, or the default tenant when none is configured.
+ public static String getBrokerTenantName(TableConfig tableConfig) {
+ String brokerTenant = tableConfig.getTenantConfig().getBroker();
+ return brokerTenant != null ? brokerTenant :
TagNameUtils.DEFAULT_TENANT_NAME;
+ }
+
+ /// Returns the effective quota config of the table, looking up the quota
defaults of its server and broker tenants
+ /// with the given provider (which returns null when a tenant has no quota
defaults).
+ @Nullable
+ public static QuotaConfig getEffectiveQuotaConfig(TableConfig tableConfig,
+ Function<String, QuotaConfig> tenantQuotaConfigProvider) {
+ String serverTenant = getServerTenantName(tableConfig);
+ String brokerTenant = getBrokerTenantName(tableConfig);
+ QuotaConfig serverTenantQuotaConfig =
tenantQuotaConfigProvider.apply(serverTenant);
+ QuotaConfig brokerTenantQuotaConfig =
+ brokerTenant.equals(serverTenant) ? serverTenantQuotaConfig :
tenantQuotaConfigProvider.apply(brokerTenant);
+ return mergeWithTenantQuotaConfigs(tableConfig, serverTenantQuotaConfig,
brokerTenantQuotaConfig);
+ }
+
+ /// Returns the effective quota config of the table given the quota defaults
of its server and broker tenants.
+ /// Returns the table's own quota config as is when neither tenant has quota
defaults.
+ @Nullable
+ public static QuotaConfig mergeWithTenantQuotaConfigs(TableConfig
tableConfig,
+ @Nullable QuotaConfig serverTenantQuotaConfig, @Nullable QuotaConfig
brokerTenantQuotaConfig) {
+ QuotaConfig tableQuotaConfig = tableConfig.getQuotaConfig();
+ if (serverTenantQuotaConfig == null && brokerTenantQuotaConfig == null) {
+ return tableQuotaConfig;
+ }
+ if (tableQuotaConfig == null) {
+ tableQuotaConfig = new QuotaConfig(null, null);
+ }
+ String storage = tableQuotaConfig.getStorage();
+ QuotaEnforcementMode storageQuotaEnforcementMode =
tableQuotaConfig.getStorageQuotaEnforcementMode();
+ if (serverTenantQuotaConfig != null) {
+ storage = firstNonNull(storage, serverTenantQuotaConfig.getStorage());
+ storageQuotaEnforcementMode =
+ firstNonNull(storageQuotaEnforcementMode,
serverTenantQuotaConfig.getStorageQuotaEnforcementMode());
Review Comment:
The mode is inherited separately from the limit, so a tenant
`storageQuotaEnforcementMode` of SHADOW or DISABLED also downgrades tables that
set their own `storage` but no mode. That includes dimension tables, whose
storage `ensureStorageQuotaConstraints` fills from
`controller.dimTable.maxSize`, so one tenant PUT can switch off the cluster's
dim-table size guard. If this is intended, please call it out in the API notes;
otherwise, consider applying the tenant mode only to limits that also come from
the tenant.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -206,9 +208,38 @@ public void initOrUpdateTableQueryQuota(TableConfig
tableConfig, ExternalView br
String tableNameWithType = tableConfig.getTableName();
LOGGER.info("Initializing rate limiter for table {}", tableNameWithType);
- // Create rate limiter if query quota config is specified.
+ // Create rate limiter if query quota config is specified, by the table or
its broker tenant.
Stat stat =
_propertyStore.getStat(constructTableConfigPath(tableNameWithType),
AccessOption.PERSISTENT);
- createOrUpdateRateLimiter(tableNameWithType, brokerResourceEV,
tableConfig.getQuotaConfig(), stat);
+ createOrUpdateRateLimiter(tableNameWithType, brokerResourceEV,
getEffectiveQuotaConfig(tableConfig), stat);
+ }
+
+ /// Rebuilds the rate limiters of the tables on the given broker tenant
served by this broker, after the quota
+ /// defaults of the tenant changed. Tenant quota defaults do not change the
table config stat version, so this must be
+ /// triggered explicitly (by a tenant config refresh message).
+ /// @param tenantName name of the tenant whose quota defaults changed.
+ public void refreshTenantQueryQuotas(String tenantName) {
+ ExternalView brokerResourceEV = getBrokerResource();
+ int numRefreshed = 0;
+ for (String physicalOrLogicalTableName : new
ArrayList<>(_rateLimiterMap.keySet())) {
+ if
(TableNameBuilder.getTableTypeFromTableName(physicalOrLogicalTableName) ==
null) {
+ // Logical tables do not inherit tenant quota defaults
+ continue;
+ }
+ TableConfig tableConfig =
ZKMetadataProvider.getTableConfig(_propertyStore, physicalOrLogicalTableName);
+ if (tableConfig != null &&
QuotaConfigUtils.getBrokerTenantName(tableConfig).equals(tenantName)) {
+ initOrUpdateTableQueryQuota(tableConfig, brokerResourceEV);
+ numRefreshed++;
+ }
+ }
+ LOGGER.info("Refreshed query rate limiters of {} tables after quota
defaults of tenant: {} changed", numRefreshed,
+ tenantName);
+ }
+
+ /// Returns the table's quota config with unset fields filled from the quota
defaults of its tenants.
+ @Nullable
+ private QuotaConfig getEffectiveQuotaConfig(TableConfig tableConfig) {
+ return QuotaConfigUtils.getEffectiveQuotaConfig(tableConfig,
+ tenantName -> ZKMetadataProvider.getTenantQuotaConfig(_propertyStore,
tenantName));
Review Comment:
Minor: the broker only uses broker-tenant defaults, but
`getEffectiveQuotaConfig` also reads the server tenant's record. When the two
tenants differ, that is an extra ZK read per table at startup and on every
tenant refresh. The controller reads the unused broker-tenant record in the
same way. A role-specific lookup would avoid both.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -691,13 +788,30 @@ public SuccessResponse deleteTenant(
break;
}
if (res.isSuccessful()) {
+ deleteTenantQuotaConfigIfTenantGone(tenantName);
return new SuccessResponse("Successfully deleted tenant " + tenantName);
}
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_TABLE_TENANT_DELETE_ERROR,
1L);
throw new ControllerApplicationException(LOGGER, "Error deleting tenant.
Reason: " + res.getMessage(),
Response.Status.INTERNAL_SERVER_ERROR);
}
+ /// Deletes the quota defaults of a tenant once it no longer exists as
either a server or a broker tenant, so that
+ /// they don't silently apply to a tenant later re-created with the same
name.
+ private void deleteTenantQuotaConfigIfTenantGone(String tenantName) {
Review Comment:
This cleanup only runs from `DELETE /tenants/{name}`. A tenant can also
disappear when its instances are untagged (`updateTags`) or dropped. The record
then stays behind and applies again if the tenant name is reused, which
contradicts "defaults are removed once the tenant no longer exists" in the PR
description. Suggest narrowing that statement, or surfacing orphaned records,
e.g. a warning on GET.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
