Jackie-Jiang commented on code in PR #17579:
URL: https://github.com/apache/pinot/pull/17579#discussion_r2824135464
##########
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:
This is not really deprecated. It is used to control if utilization checker
(the entire module) is enabled
##########
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:
(minor)
```suggestion
UtilizationChecker.CheckResult isResourceUtilizationWithinLimits =
```
##########
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:
This is backward incompatible. It should be `true` by default
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -636,7 +643,7 @@ private void setUpPinotController() {
_helixResourceManager, _config);
_diskUtilizationChecker = new
DiskUtilizationChecker(_helixResourceManager, _config);
Review Comment:
You may change this to local variable, and remove the binding to
DiskUtilizationChecker. That is legacy code, and is replaced with
`ResourceUtilizationManager`
##########
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
public static final String ENABLE_RESOURCE_UTILIZATION_CHECK =
"controller.enable.resource.utilization.check";
+ // Explicitly enables all resource utilization checkers
+ public static final String ENABLE_ALL_RESOURCE_UTILIZATION_CHECKERS =
+ "controller.enable.resource.utilization.checkers.all";
+ // If controller.enable.resource.utilization.checkers.all = false, each
individual utilization checker can be enabled.
+ // When a new resource utilization checker is added, a new config must be
added to have the option to specifically
+ // enable/disable it.
+ public static final String ENABLE_DISK_UTILIZATION_CHECKER =
+ "controller.enable.resource.utilization.checkers.disk";
Review Comment:
```suggestion
"controller.enable.disk.utilization.checker";
```
##########
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
public static final String ENABLE_RESOURCE_UTILIZATION_CHECK =
"controller.enable.resource.utilization.check";
+ // Explicitly enables all resource utilization checkers
+ public static final String ENABLE_ALL_RESOURCE_UTILIZATION_CHECKERS =
+ "controller.enable.resource.utilization.checkers.all";
Review Comment:
```suggestion
"controller.enable.all.resource.utilization.checkers";
```
##########
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:
Don't modify this method because it is used to plug-in checker. You can add
the check underlying before constructing `DiskUtilizationChecker`
--
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]
