Copilot commented on code in PR #12401:
URL: https://github.com/apache/cloudstack/pull/12401#discussion_r2681125225
##########
engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql:
##########
@@ -25,3 +25,6 @@ CALL
`cloud_usage`.`IDEMPOTENT_ADD_COLUMN`('cloud_usage.usage_event','vm_id', 'b
-- Add vm_id column to cloud_usage.usage_volume table
CALL `cloud_usage`.`IDEMPOTENT_ADD_COLUMN`('cloud_usage.usage_volume','vm_id',
'bigint UNSIGNED NULL COMMENT "VM ID associated with the volume usage"');
+
+-- Drops the unused "backup_interval_type" column of the "cloud.backups" table
+ALTER TABLE `cloud`.`backups` DROP COLUMN `backup_interval_type`;
Review Comment:
The migration lacks a safety check (e.g., IF EXISTS or conditional check) to
prevent failures if the column has already been dropped in a previous migration
attempt. Consider using a procedure or adding a check to ensure idempotency.
```suggestion
ALTER TABLE `cloud`.`backups` DROP COLUMN IF EXISTS `backup_interval_type`;
```
##########
engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupScheduleDaoImpl.java:
##########
@@ -88,21 +82,18 @@ public List<BackupScheduleVO> getSchedulesToExecute(Date
currentTimestamp) {
return listBy(sc);
}
+ @DB
@Override
- public BackupScheduleResponse newBackupScheduleResponse(BackupSchedule
schedule) {
- VMInstanceVO vm =
vmInstanceDao.findByIdIncludingRemoved(schedule.getVmId());
- BackupScheduleResponse response = new BackupScheduleResponse();
- response.setId(schedule.getUuid());
- response.setVmId(vm.getUuid());
- response.setVmName(vm.getHostName());
- response.setIntervalType(schedule.getScheduleType());
- response.setSchedule(schedule.getSchedule());
- response.setTimezone(schedule.getTimezone());
- response.setMaxBackups(schedule.getMaxBackups());
- if (schedule.getQuiesceVM() != null) {
- response.setQuiesceVM(schedule.getQuiesceVM());
+ public boolean remove(Long id) {
+ String sql = "UPDATE backups SET backup_schedule_id = NULL WHERE
backup_schedule_id = ?";
+ TransactionLegacy transaction = TransactionLegacy.currentTxn();
+ try {
+ PreparedStatement preparedStatement =
transaction.prepareAutoCloseStatement(sql);
+ preparedStatement.setLong(1, id);
+ preparedStatement.executeUpdate();
+ return super.remove(id);
+ } catch (SQLException e) {
+ return false;
}
Review Comment:
The SQLException is caught but not logged or handled beyond returning false.
This makes debugging difficult if the cleanup fails. Consider logging the
exception with details about which schedule ID failed to be cleaned up.
##########
engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupScheduleDaoImpl.java:
##########
@@ -88,21 +82,18 @@ public List<BackupScheduleVO> getSchedulesToExecute(Date
currentTimestamp) {
return listBy(sc);
}
+ @DB
@Override
- public BackupScheduleResponse newBackupScheduleResponse(BackupSchedule
schedule) {
- VMInstanceVO vm =
vmInstanceDao.findByIdIncludingRemoved(schedule.getVmId());
- BackupScheduleResponse response = new BackupScheduleResponse();
- response.setId(schedule.getUuid());
- response.setVmId(vm.getUuid());
- response.setVmName(vm.getHostName());
- response.setIntervalType(schedule.getScheduleType());
- response.setSchedule(schedule.getSchedule());
- response.setTimezone(schedule.getTimezone());
- response.setMaxBackups(schedule.getMaxBackups());
- if (schedule.getQuiesceVM() != null) {
- response.setQuiesceVM(schedule.getQuiesceVM());
+ public boolean remove(Long id) {
+ String sql = "UPDATE backups SET backup_schedule_id = NULL WHERE
backup_schedule_id = ?";
+ TransactionLegacy transaction = TransactionLegacy.currentTxn();
+ try {
+ PreparedStatement preparedStatement =
transaction.prepareAutoCloseStatement(sql);
+ preparedStatement.setLong(1, id);
+ preparedStatement.executeUpdate();
+ return super.remove(id);
+ } catch (SQLException e) {
Review Comment:
If the backup reference cleanup succeeds but super.remove(id) fails, the
backups will have NULL schedule references but the schedule will still exist.
Consider wrapping both operations in a transaction to ensure atomicity, or
handle the super.remove() failure case separately.
```suggestion
transaction.start();
try {
PreparedStatement preparedStatement =
transaction.prepareAutoCloseStatement(sql);
preparedStatement.setLong(1, id);
preparedStatement.executeUpdate();
boolean removed = super.remove(id);
if (!removed) {
transaction.rollback();
return false;
}
transaction.commit();
return true;
} catch (SQLException e) {
transaction.rollback();
```
--
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]
