mluvin-stripe commented on code in PR #17579:
URL: https://github.com/apache/pinot/pull/17579#discussion_r2855285095
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -379,8 +379,16 @@ private void setupHelixClusterConstraints() {
MAX_STATE_TRANSITIONS_PER_RESOURCE, constraintItemResource);
}
- protected void addUtilizationChecker(UtilizationChecker utilizationChecker) {
- _utilizationCheckers.add(utilizationChecker);
+ protected void maybeAddUtilizationChecker(UtilizationChecker
utilizationChecker,
+ boolean isSpecificUtilizationCheckerEnabled) {
+ // Add utilization checker if:
+ // 1. All resource utilization checkers are enabled, OR
+ // 2. This specific utilization checker is enabled, OR
+ // 3. The legacy resource utilization check config is enabled (backwards
compatibility)
+ if (_config.isAllResourceUtilizationCheckersEnabled() ||
isSpecificUtilizationCheckerEnabled
+ || _config.isResourceUtilizationCheckEnabled()) {
+ _utilizationCheckers.add(utilizationChecker);
+ }
Review Comment:
@Jackie-Jiang i'll change this logic (will be in a separate function as
well) to just be
```suggestion
if (_config.isAllResourceUtilizationCheckersEnabled() ||
isSpecificUtilizationCheckerEnabled) {
_utilizationCheckers.add(utilizationChecker);
}
```
which will be backwards compatible since i'll change the new
`controller.enable.resource.utilization.checkers.all` to be `true` by default
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -403,6 +411,27 @@ private SuccessResponse uploadSegment(@Nullable String
tableName, TableType tabl
SegmentValidationUtils.checkStorageQuota(segmentName,
segmentSizeInBytes, untarredSegmentSizeInBytes, tableConfig,
_storageQuotaChecker);
+ // Perform resource utilization checks
+ UtilizationChecker.CheckResult isDiskUtilizationWithinLimits =
Review Comment:
thanks, missed renaming this
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -379,8 +379,16 @@ private void setupHelixClusterConstraints() {
MAX_STATE_TRANSITIONS_PER_RESOURCE, constraintItemResource);
}
- protected void addUtilizationChecker(UtilizationChecker utilizationChecker) {
- _utilizationCheckers.add(utilizationChecker);
+ protected void maybeAddUtilizationChecker(UtilizationChecker
utilizationChecker,
Review Comment:
got it. i'll make a separate `shouldAddUtilizationChecker` method instead
and call that before constructing the `DiskUtilizationChecker`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -377,6 +388,8 @@ public static long getRandomInitialDelayInSeconds() {
public static final int DEFAULT_DISK_UTILIZATION_CHECK_TIMEOUT_MS = 30_000;
public static final String DEFAULT_DISK_UTILIZATION_PATH =
"/home/pinot/data";
public static final boolean DEFAULT_ENABLE_RESOURCE_UTILIZATION_CHECK =
false;
+ public static final boolean DEFAULT_ENABLE_ALL_RESOURCE_UTILIZATION_CHECKERS
= false;
+ public static final boolean DEFAULT_ENABLE_DISK_UTILIZATION_CHECKER = false;
Review Comment:
i see what you mean now given
https://github.com/apache/pinot/pull/17579/changes#r2855262308. will change
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -342,7 +343,17 @@ public static long getRandomInitialDelayInSeconds() {
public static final String DISK_UTILIZATION_THRESHOLD =
"controller.disk.utilization.threshold"; // 0 < threshold < 1
public static final String DISK_UTILIZATION_CHECK_TIMEOUT_MS =
"controller.disk.utilization.check.timeoutMs";
public static final String DISK_UTILIZATION_PATH =
"controller.disk.utilization.path";
+ // Deprecated in favor of controller.enable.resource.utilization.checkers.all
+ @Deprecated
Review Comment:
I see what you're saying. This config should remain unchanged, as it's used
[here](https://github.com/mluvin-stripe/pinot/blob/234d38239643914c5c193e4f297e4d4f2432c1a7/pinot-controller/src/main/java/org/apache/pinot/controller/validation/ResourceUtilizationManager.java#L51)
to determine whether or not to even run the utilization checkers.
the new `controller.enable.all.resource.utilization.checkers` will control
whether to plug in all the checkers;
`controller.enable.disk.utilization.checker` controls just the disk utilization
checker if the `all` config is 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.
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]
