[GitHub] [cloudstack] nvazquez commented on a change in pull request #4121: server: fix TransactionLegacy DB connection leaks due to DB switching by B&R thread

[GitBox <gi...@apache.org>]

nvazquez commented on a change in pull request #4121:
URL: https://github.com/apache/cloudstack/pull/4121#discussion_r434313711



##########
File path: engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDaoImpl.java
##########
@@ -19,69 +19,71 @@
 
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
+import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.TimeZone;
 
-import org.apache.cloudstack.backup.Backup;
 import org.apache.log4j.Logger;
 import org.springframework.stereotype.Component;
 
+import com.cloud.exception.CloudException;
 import com.cloud.usage.UsageBackupVO;
 import com.cloud.utils.DateUtil;
 import com.cloud.utils.db.GenericDaoBase;
-import com.cloud.utils.db.QueryBuilder;
 import com.cloud.utils.db.SearchCriteria;
-import com.cloud.utils.db.Transaction;
-import com.cloud.utils.db.TransactionCallback;
 import com.cloud.utils.db.TransactionLegacy;
-import com.cloud.utils.db.TransactionStatus;
-import com.cloud.vm.VirtualMachine;
 
 @Component
 public class UsageBackupDaoImpl extends GenericDaoBase<UsageBackupVO, Long> implements UsageBackupDao {
     public static final Logger LOGGER = Logger.getLogger(UsageBackupDaoImpl.class);
-    protected static final String GET_USAGE_RECORDS_BY_ACCOUNT = "SELECT id, zone_id, account_id, domain_id, vm_id, backup_offering_id, size, protected_size, created, removed FROM cloud_usage.usage_backup WHERE " +
+    protected static final String UPDATE_DELETED = "UPDATE usage_backup SET removed = ? WHERE account_id = ? AND vm_id = ? and removed IS NULL";
+    protected static final String GET_USAGE_RECORDS_BY_ACCOUNT = "SELECT id, zone_id, account_id, domain_id, vm_id, backup_offering_id, size, protected_size, created, removed FROM usage_backup WHERE " +
             " account_id = ? AND ((removed IS NULL AND created <= ?) OR (created BETWEEN ? AND ?) OR (removed BETWEEN ? AND ?) " +
             " OR ((created <= ?) AND (removed >= ?)))";
 
     @Override
-    public void updateMetrics(final VirtualMachine vm, Backup.Metric metric) {
-        boolean result = Transaction.execute(TransactionLegacy.USAGE_DB, new TransactionCallback<Boolean>() {
-            @Override
-            public Boolean doInTransaction(final TransactionStatus status) {
-                final QueryBuilder<UsageBackupVO> qb = QueryBuilder.create(UsageBackupVO.class);
-                qb.and(qb.entity().getVmId(), SearchCriteria.Op.EQ, vm.getId());
-                final UsageBackupVO entry = findOneBy(qb.create());
-                if (entry == null) {
-                    return false;
-                }
-                entry.setSize(metric.getBackupSize());
-                entry.setProtectedSize(metric.getDataSize());
-                return update(entry.getId(), entry);
+    public void updateMetrics(final Long vmId, final Long size, final Long virtualSize) {
+        TransactionLegacy txn = TransactionLegacy.open(TransactionLegacy.USAGE_DB);
+        try {
+            SearchCriteria<UsageBackupVO> sc = this.createSearchCriteria();
+            sc.addAnd("vmId", SearchCriteria.Op.EQ, vmId);
+            UsageBackupVO vo = findOneBy(sc);
+            if (vo != null) {
+                vo.setSize(size);
+                vo.setProtectedSize(virtualSize);
+                update(vo.getId(), vo);
             }
-        });
-        if (!result) {
-            LOGGER.trace("Failed to update backup metrics for VM ID: " + vm.getId());
+        } catch (final Exception e) {
+            LOGGER.error("Error updating backup metrics: " + e.getMessage(), e);
+        } finally {
+            txn.close();

Review comment:
       +1 to explicitly close the transaction. The `Transaction.execute()` method is being used widely over the codebase, looks strange that closing has not been handled on it

##########
File path: server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
##########
@@ -1022,31 +1018,23 @@ protected void runInContext() {
                         continue;
                     }
 
-                    // Sync backup usage metrics
                     final Map<VirtualMachine, Backup.Metric> metrics = backupProvider.getBackupMetrics(dataCenter.getId(), new ArrayList<>(vms));
-                    final GlobalLock syncBackupMetricsLock = GlobalLock.getInternLock("BackupSyncTask_metrics_zone_" + dataCenter.getId());

Review comment:
       Not locking now, how are the metrics update and backup sync affected by this?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org