Tarun-kishore commented on code in PR #20235:
URL: https://github.com/apache/pinot/pull/20235#discussion_r4546370359


##########
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:
   Changed: PUT now replaces only the roles set in the body, via a 
compare-and-set update. Added `DELETE ?type=server|broker` to remove one role.



##########
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:
   Agreed. A tenant mode now only applies to tables inheriting the tenant 
limit, so a table with its own limit (including dimension tables) keeps its own 
mode. A tenant mode without its limit is rejected.



-- 
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]

Reply via email to