Jackie-Jiang commented on code in PR #13584:
URL: https://github.com/apache/pinot/pull/13584#discussion_r1681811635
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java:
##########
@@ -75,9 +80,12 @@ public static QuotaCheckerResponse failure(String msg) {
* Returns whether the new added segment is within the storage quota.
*/
public QuotaCheckerResponse isSegmentStorageWithinQuota(TableConfig
tableConfig, String segmentName,
- long segmentSizeInBytes, int timeoutMs)
+ long segmentSizeInBytes)
throws InvalidConfigException {
- Preconditions.checkArgument(timeoutMs > 0, "Timeout value must be > 0,
input: %s", timeoutMs);
+ if (!_controllerConf.getEnableStorageQuotaCheck()) {
+ return success("Storage quota check is disabled, skipping the check");
+ }
+ Preconditions.checkArgument(_timeout > 0, "Timeout value must be > 0,
input: %s", _timeout);
Review Comment:
Move this check to the constructor?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/StorageQuotaChecker.java:
##########
@@ -205,4 +213,48 @@ public QuotaCheckerResponse
isSegmentStorageWithinQuota(TableConfig tableConfig,
return failure(message);
}
}
+
+ public boolean isTableStorageQuotaExceeded(TableConfig tableConfig) {
Review Comment:
Should we also check if quota check is enabled?
Consider extracting the common part for `isSegmentStorageWithinQuota()` and
this method
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1696,6 +1705,20 @@ private IdealState updatePauseStatusInIdealState(String
tableNameWithType, boole
return updatedIdealState;
}
+ public boolean updateStorageQuotaExceededInIdealState(String
tableNameWithType, boolean quotaExceeded) {
+ IdealState is = getIdealState(tableNameWithType);
+ if (is.getRecord().getBooleanField(IS_QUOTA_EXCEEDED, false) !=
quotaExceeded) {
+ HelixHelper.updateIdealState(_helixManager, tableNameWithType,
idealState -> {
+ ZNRecord znRecord = idealState.getRecord();
+ znRecord.setSimpleField(IS_QUOTA_EXCEEDED,
Boolean.valueOf(quotaExceeded).toString());
+ return new IdealState(znRecord);
+ }, RetryPolicies.noDelayRetryPolicy(1));
Review Comment:
Why not allow retry here?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -108,8 +110,20 @@ protected void processTable(String tableNameWithType,
Context context) {
if (context._runSegmentLevelValidation) {
runSegmentLevelValidation(tableConfig, streamConfig);
}
+
+ // IS is updated in below cases
+ // Case 1 -> Table is exceeding storage quota but IS still has
"isQuotaExceeded" as false
+ // This will help prevent consuming segment creation through this task
upon manual zk changes to
+ // "isQuotaExceeded" in IS.
+ // Case 2 -> Table is within quota limits now but IS still has
"isQuotaExceeded" as true
+ // This will help resume the table consumption once the quota is available
due to either quota increase or
+ // segment deletion.
+ // In which case we need to pass "recreateDeletedConsumingSegment" as true
to "ensureAllPartitionsConsuming" below.
+ boolean idealStateUpdated =
_llcRealtimeSegmentManager.updateStorageQuotaExceededInIdealState(tableNameWithType,
+ _storageQuotaChecker.isTableStorageQuotaExceeded(tableConfig));
+
_llcRealtimeSegmentManager.ensureAllPartitionsConsuming(tableConfig,
streamConfig,
- context._recreateDeletedConsumingSegment, context._offsetCriteria);
+ context._recreateDeletedConsumingSegment || idealStateUpdated,
context._offsetCriteria);
Review Comment:
We should recreate deleted consuming segment only with case 2 above?
--
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
