mcvsubbu commented on code in PR #13584:
URL: https://github.com/apache/pinot/pull/13584#discussion_r1755282957
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java:
##########
@@ -43,14 +44,19 @@ public class StorageQuotaChecker {
private final ControllerMetrics _controllerMetrics;
private final LeadControllerManager _leadControllerManager;
private final PinotHelixResourceManager _pinotHelixResourceManager;
+ private final ControllerConf _controllerConf;
+ private final int _timeout;
public StorageQuotaChecker(TableSizeReader tableSizeReader,
ControllerMetrics controllerMetrics, LeadControllerManager
leadControllerManager,
- PinotHelixResourceManager pinotHelixResourceManager) {
+ PinotHelixResourceManager pinotHelixResourceManager, ControllerConf
controllerConf) {
_tableSizeReader = tableSizeReader;
_controllerMetrics = controllerMetrics;
_leadControllerManager = leadControllerManager;
_pinotHelixResourceManager = pinotHelixResourceManager;
+ _controllerConf = controllerConf;
Review Comment:
Nit. I may just pick up whatever is needed from config and not keep the
entire config object here. In this case, an `_isEnabled` boolean will suffice?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java:
##########
@@ -43,14 +44,19 @@ public class StorageQuotaChecker {
private final ControllerMetrics _controllerMetrics;
private final LeadControllerManager _leadControllerManager;
private final PinotHelixResourceManager _pinotHelixResourceManager;
+ private final ControllerConf _controllerConf;
+ private final int _timeout;
Review Comment:
I think it is useful to call it `_timeoutMs` since it improves readability.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -108,8 +112,32 @@ protected void processTable(String tableNameWithType,
Context context) {
if (context._runSegmentLevelValidation) {
runSegmentLevelValidation(tableConfig, streamConfig);
}
+
+ // Keeps the table paused/unpaused based on storage quota validation.
+ // Skips updating the pause state if table is paused by admin
+ PauseState pauseState = computePauseState(tableNameWithType);
+
_llcRealtimeSegmentManager.ensureAllPartitionsConsuming(tableConfig,
streamConfig,
- context._recreateDeletedConsumingSegment, context._offsetCriteria);
+ context._recreateDeletedConsumingSegment || !pauseState.isPaused(),
context._offsetCriteria);
Review Comment:
Please change this to _not_ call `ensureAllPartitionsConsuming` if the
table is paused (or storage is exceeeded) rather than overloading the paused
boolean with recreation of deleted segment.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
