Copilot commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2333008501
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -946,13 +946,13 @@ public boolean configure(String name, Map<String, Object>
params) throws Configu
return true;
}
Review Comment:
The method signature change from `isDisabled(final Long zoneId)` to
`isDisabled()` is a breaking change that removes the zone parameter. This could
break existing callers that pass a zone ID. Consider deprecating the old method
first or ensuring all callers are updated in this PR.
```suggestion
/**
* @deprecated Use {@link #isDisabled()} instead. This method will be
removed in a future release.
*/
@Deprecated
public boolean isDisabled(final Long zoneId) {
return isDisabled();
}
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1236,12 +1236,11 @@ protected void runInContext() {
if (logger.isTraceEnabled()) {
logger.trace("Backup sync background task is running...");
}
+ if (isDisabled()) {
+ logger.debug("Backup Sync Task is not enabled. Skipping!");
+ return;
+ }
for (final DataCenter dataCenter :
dataCenterDao.listAllZones()) {
Review Comment:
Moving the `isDisabled()` check outside the zone loop improves performance
by avoiding redundant global configuration checks for each zone. However, this
changes the behavior from per-zone checking to a single global check that exits
early.
--
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]
