Copilot commented on code in PR #13571:
URL: https://github.com/apache/cloudstack/pull/13571#discussion_r3546424631
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -1000,53 +1035,24 @@ private boolean isDeletePending(Backup backup) {
}
/**
- * Return the live (not delete-pending, not Removed) children of {@code
parent} within the
+ * Whether there are any live (not delete-pending, not Removed) children
of {@code parent} within the
* same chain. Equivalent to "incrementals whose parent_backup_id points
at parent".
*/
Review Comment:
The hasLiveChildren() Javadoc still describes the old behavior (immediate
children via PARENT_BACKUP_ID). The implementation now checks for any live
descendants via CHAIN_POSITION, so the comment is misleading and makes the
chain-delete logic harder to reason about.
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -1000,53 +1035,24 @@ private boolean isDeletePending(Backup backup) {
}
/**
- * Return the live (not delete-pending, not Removed) children of {@code
parent} within the
+ * Whether there are any live (not delete-pending, not Removed) children
of {@code parent} within the
* same chain. Equivalent to "incrementals whose parent_backup_id points
at parent".
*/
- private List<Backup> findLiveChildren(Backup parent) {
- String parentUuid = parent.getUuid();
- String chainId = readDetail(parent, NASBackupChainKeys.CHAIN_ID);
- if (parentUuid == null || chainId == null) {
- return Collections.emptyList();
- }
- List<Backup> children = new ArrayList<>();
- for (Backup b : backupDao.listByVmId(null, parent.getVmId())) {
- if (b.getId() == parent.getId()) {
- continue;
- }
- if (!chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
- continue;
- }
- if (!parentUuid.equals(readDetail(b,
NASBackupChainKeys.PARENT_BACKUP_ID))) {
- continue;
- }
- if (isDeletePending(b)) {
- // Tombstoned children don't keep us alive — they're already
on the way out.
- continue;
- }
- children.add(b);
- }
- return children;
- }
-
- /**
- * Look up this backup's immediate parent in the chain (by {@code
PARENT_BACKUP_ID}).
- * Returns {@code null} if this is the full (no parent) or the parent row
is gone.
- *
- * <p>Prefer {@link #getChainOrderedLeafToRoot(Backup)} when walking the
whole chain —
- * this method hits the DB on each call and is O(N²) when used in a loop.
- */
- private Backup findChainParent(Backup backup) {
- String parentUuid = readDetail(backup,
NASBackupChainKeys.PARENT_BACKUP_ID);
- if (parentUuid == null || parentUuid.isEmpty()) {
- return null;
+ private boolean hasLiveChildren(Backup backup) {
+ String chainId = readDetail(backup, NASBackupChainKeys.CHAIN_ID);
+ if (chainId == null) {
+ return false;
}
+ int position = chainPosition(backup);
for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
- if (parentUuid.equals(b.getUuid())) {
- return b;
+ if (b.getId() == backup.getId() || !chainId.equals(readDetail(b,
NASBackupChainKeys.CHAIN_ID))) {
+ continue;
+ }
Review Comment:
hasLiveChildren() relies on CHAIN_POSITION ordering, but chainPosition()
returns Integer.MAX_VALUE when CHAIN_POSITION is missing/invalid. In that case
this method can incorrectly conclude there are no live descendants (because no
position is greater than MAX_VALUE) and physically delete a backup that still
has dependents in the chain.
--
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]
