[Feature] Support Cluster Snapshot Backup: deletion control (part4) #54980

srlch · 2025-01-13T02:42:13Z

What I'm doing:

This is part 4 for Support Cluster Snapshot Backup
In this pr, we support deletion control to keep the files in BE/CN side needed by the
current cluster snapshot. The basic idea is following:

Introduce the deletion valid time stamp, which is the previous created time for automated
cluster snapshot, called ValidDeletionTimeMs
Use ValidDeletionTimeMs for vacuum.
Use ValidDeletionTimeMs to filter for partition/table in recycleBin
Use ValidDeletionTimeMs to filter alter job

Fixes #53867
#53867

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

starrocks-cr · 2025-01-13T02:51:23Z

fe/fe-core/src/main/java/com/starrocks/lake/snapshot/ClusterSnapshotMgr.java

+        }
+        return valid;
+    }
+
    public TClusterSnapshotJobsResponse getAllJobsInfo() {
        TClusterSnapshotJobsResponse response = new TClusterSnapshotJobsResponse();
        for (Map.Entry<Long, ClusterSnapshotJob> entry : historyAutomatedSnapshotJobs.entrySet()) {


The most risky bug in this code is:
Incorrect conversion of time units leading to potential logic errors.

You can modify the code like this:

valid = (alterJob.getFinishedTimeMs() < getValidDeletionTimeSecByAutomatedSnapshot() * 1000L);

Explanation: The multiplication of getValidDeletionTimeSecByAutomatedSnapshot() by 1000 should explicitly use a long literal (1000L) to ensure the calculation handles large numbers properly, matching the type of getFinishedTimeMs() which likely returns milliseconds as a long.

starrocks-cr · 2025-01-13T02:51:29Z

fe/fe-core/src/main/java/com/starrocks/catalog/CatalogRecycleBin.java

@@ -635,7 +651,7 @@ private synchronized void disableRecoverPartitionWithSameName(long dbId, long ta
                continue;
            }
            partitionInfo.setRecoverable(false);
-            idToRecycleTime.replace(partitionInfo.getPartition().getId(), 0L);
+            idToRecycleTime.replace(partitionInfo.getPartition().getId(), System.currentTimeMillis());
            break;
        }
    }


The most risky bug in this code is:
Changing the recycle time to System.currentTimeMillis() may prevent immediate deletion of non-recoverable tables or partitions, which should be set to zero.

You can modify the code like this:

// Keep original behavior for non-recoverable items by setting their recycle time to 0. idToRecycleTime.put(table.getId(), !recoverable ? 0 : System.currentTimeMillis()); // Update other relevant parts where non-recoverable state should set time to 0 similarly.

xiangguangyxg · 2025-01-13T07:32:38Z

fe/fe-core/src/main/java/com/starrocks/catalog/CatalogRecycleBin.java

+        if (idToRecycleTableInfo.get(id) != null) {
+            recoverable = idToRecycleTableInfo.get(id).isRecoverable();
+        } else if (idToPartition.get(id) != null) {
+            recoverable = idToPartition.get(id).isRecoverable();
+        }


duplicated get()

xiangguangyxg · 2025-01-13T07:43:53Z

fe/fe-core/src/main/java/com/starrocks/lake/snapshot/ClusterSnapshotMgr.java

@@ -52,6 +53,8 @@ public class ClusterSnapshotMgr implements GsonPostProcessable {
    @SerializedName(value = "historyAutomatedSnapshotJobs")
    private TreeMap<Long, ClusterSnapshotJob> historyAutomatedSnapshotJobs = new TreeMap<>();

+    private long previousAutomatedSnapshotCreatedTimsMs = 0;


not needed ? there is a createdTimeMs in snapshot

xiangguangyxg · 2025-01-13T09:15:19Z

fe/fe-core/src/main/java/com/starrocks/lake/snapshot/ClusterSnapshotMgr.java

+
+        boolean findFirstSuccess = false;
+        long previousAutomatedSnapshotCreatedTimsMs = 0;
+        for (Long key : historyAutomatedSnapshotJobs.descendingKeySet()) {


using descendingMap is better ?

xiangguangyxg · 2025-01-14T08:29:11Z

fe/fe-core/src/main/java/com/starrocks/catalog/CatalogRecycleBin.java

+        if (!idToRecycleTime.containsKey(id)) {
+            return true;
+        }
+        return idToRecycleTime.get(id) < GlobalStateMgr.getCurrentState()


containsKey() and get() are duplicated looking up

xiangguangyxg · 2025-01-14T08:29:44Z

fe/fe-core/src/main/java/com/starrocks/lake/snapshot/ClusterSnapshotJob.java

@@ -105,6 +105,10 @@ public boolean isUnFinishedState() {
               state == ClusterSnapshotJobState.FINISHED;
    }

+    public boolean isSuccess() {


Suggested change

public boolean isSuccess() {

public boolean isFinished() {

xiangguangyxg · 2025-01-14T08:31:43Z

fe/fe-core/src/main/java/com/starrocks/catalog/CatalogRecycleBin.java

+            return 0;
+        }
+
+        return idToRecycleTime.get(id);


if a id not in idToRecycleTime, it will throw exception ?

In the original logic, the caller will ensure that the key should be existed

could simply handle this exception ? just return 0 is ok ?

Signed-off-by: srlch <[email protected]>

sonarqubecloud · 2025-01-17T04:50:38Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2025-01-17T06:27:47Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2025-01-17T06:28:21Z

[FE Incremental Coverage Report]

✅ pass : 82 / 86 (95.35%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/catalog/CatalogRecycleBin.java	27	31	87.10%	[305, 348, 364, 401]
🔵	com/starrocks/lake/snapshot/ClusterSnapshotMgr.java	30	30	100.00%	[]
🔵	com/starrocks/lake/snapshot/ClusterSnapshotJob.java	8	8	100.00%	[]
🔵	com/starrocks/alter/AlterHandler.java	3	3	100.00%	[]
🔵	com/starrocks/lake/vacuum/AutovacuumDaemon.java	3	3	100.00%	[]
🔵	com/starrocks/lake/snapshot/ClusterSnapshot.java	8	8	100.00%	[]
🔵	com/starrocks/lake/StarMgrMetaSyncer.java	3	3	100.00%	[]

github-actions · 2025-01-17T06:28:37Z

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

xiangguangyxg · 2025-01-17T06:51:57Z

fe/fe-core/src/main/java/com/starrocks/lake/snapshot/ClusterSnapshotMgr.java

            historyAutomatedSnapshotJobs.pollFirstEntry();
        }
        historyAutomatedSnapshotJobs.put(job.getId(), job);
    }

+    public synchronized long getValidDeletionTimeMsByAutomatedSnapshot() {


Suggested change

public synchronized long getValidDeletionTimeMsByAutomatedSnapshot() {

public synchronized long getSafeDeletionTimeMs() {

better to add another function：
boolean isDeletiionSafeToExecute(long deletionCreatedTimeMs)

will fix in next pr

xiangguangyxg · 2025-01-17T08:09:51Z

fe/fe-core/src/main/java/com/starrocks/lake/snapshot/ClusterSnapshotMgr.java

+        return previousAutomatedSnapshotCreatedTimsMs;
+    }
+
+    public synchronized boolean checkValidDeletionForTableFromAlterJob(long tableId) {


Suggested change

public synchronized boolean checkValidDeletionForTableFromAlterJob(long tableId) {

public synchronized boolean isTableSafeToDeleteTablet(long tableId) { ?

xiangguangyxg · 2025-01-17T08:11:18Z

fe/fe-core/src/main/java/com/starrocks/catalog/CatalogRecycleBin.java

+            return 0;
+        }
+
+        return idToRecycleTime.get(id);


could simply handle this exception ? just return 0 is ok ?

srlch · 2025-01-17T08:16:53Z

https://github.com/Mergifyio backport branch-3.4

mergify · 2025-01-17T08:17:02Z

backport branch-3.4

✅ Backports have been created

#55206 [Feature] Support Cluster Snapshot Backup: deletion control (part4) (backport #54980) has been created for branch branch-3.4

…54980) Signed-off-by: srlch <[email protected]> (cherry picked from commit 5cc4e6a)

…backport #54980) (#55206) Co-authored-by: srlch <[email protected]>

srlch requested a review from a team as a code owner January 13, 2025 02:42

mergify bot assigned srlch Jan 13, 2025

starrocks-cr bot reviewed Jan 13, 2025

View reviewed changes

wanpengfei-git added the META-REVIEW label Jan 13, 2025

wanpengfei-git requested a review from a team January 13, 2025 03:45

xiangguangyxg reviewed Jan 13, 2025

View reviewed changes

srlch force-pushed the cluster_snapshot_part_4 branch from 21bf08a to 4683647 Compare January 13, 2025 10:35

xiangguangyxg approved these changes Jan 13, 2025

View reviewed changes

srlch force-pushed the cluster_snapshot_part_4 branch 3 times, most recently from 7109613 to 3ec6546 Compare January 14, 2025 06:23

xiangguangyxg reviewed Jan 14, 2025

View reviewed changes

srlch force-pushed the cluster_snapshot_part_4 branch 2 times, most recently from c66d6ee to 3f4cbce Compare January 15, 2025 10:55

srlch added 7 commits January 17, 2025 10:24

[Feature] Support Cluster Snapshot Backup: deletion control (part4)

90bc0cf

Signed-off-by: srlch <[email protected]>

fix

3015413

Signed-off-by: srlch <[email protected]>

fix

c411798

Signed-off-by: srlch <[email protected]>

fix

5faa3e9

Signed-off-by: srlch <[email protected]>

fix

076688a

Signed-off-by: srlch <[email protected]>

fix

a6bca3e

Signed-off-by: srlch <[email protected]>

fix

676ade4

Signed-off-by: srlch <[email protected]>

srlch force-pushed the cluster_snapshot_part_4 branch from 3f4cbce to 676ade4 Compare January 17, 2025 02:24

srlch added 2 commits January 17, 2025 10:32

fix

cc37675

Signed-off-by: srlch <[email protected]>

fix

82ed7d4

Signed-off-by: srlch <[email protected]>

gengjun-git approved these changes Jan 17, 2025

View reviewed changes

wyb approved these changes Jan 17, 2025

View reviewed changes

wyb merged commit 5cc4e6a into StarRocks:main Jan 17, 2025
49 checks passed

github-actions bot added the version:3.5 label Jan 17, 2025

wanpengfei-git mentioned this pull request Jan 17, 2025

shared-data mode support backup restore though automated snapshot #53867

Open

9 tasks

xiangguangyxg reviewed Jan 17, 2025

View reviewed changes

mergify bot pushed a commit that referenced this pull request Jan 17, 2025

[Feature] Support Cluster Snapshot Backup: deletion control (part4) (#…

7b13c9c

…54980) Signed-off-by: srlch <[email protected]> (cherry picked from commit 5cc4e6a)

mergify bot mentioned this pull request Jan 17, 2025

[Feature] Support Cluster Snapshot Backup: deletion control (part4) (backport #54980) #55206

Merged

18 tasks

wanpengfei-git pushed a commit that referenced this pull request Jan 20, 2025

[Feature] Support Cluster Snapshot Backup: deletion control (part4) (…

e34b448

…backport #54980) (#55206) Co-authored-by: srlch <[email protected]>

github-actions bot added the 3.4-merged label Jan 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Support Cluster Snapshot Backup: deletion control (part4) #54980

[Feature] Support Cluster Snapshot Backup: deletion control (part4) #54980

srlch commented Jan 13, 2025 •

edited

Loading

starrocks-cr bot Jan 13, 2025

starrocks-cr bot Jan 13, 2025

xiangguangyxg Jan 13, 2025 •

edited

Loading

srlch Jan 13, 2025

xiangguangyxg Jan 13, 2025

xiangguangyxg Jan 13, 2025

srlch Jan 13, 2025

srlch Jan 13, 2025

xiangguangyxg Jan 14, 2025

srlch Jan 17, 2025

xiangguangyxg Jan 14, 2025

srlch Jan 17, 2025

xiangguangyxg Jan 14, 2025

srlch Jan 17, 2025

xiangguangyxg Jan 17, 2025

sonarqubecloud bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

xiangguangyxg Jan 17, 2025

xiangguangyxg Jan 17, 2025

srlch Jan 17, 2025

xiangguangyxg Jan 17, 2025

xiangguangyxg Jan 17, 2025

srlch commented Jan 17, 2025

mergify bot commented Jan 17, 2025 •

edited

Loading

	public synchronized long getValidDeletionTimeMsByAutomatedSnapshot() {
	public synchronized long getSafeDeletionTimeMs() {

	public synchronized boolean checkValidDeletionForTableFromAlterJob(long tableId) {
	public synchronized boolean isTableSafeToDeleteTablet(long tableId) { ?

[Feature] Support Cluster Snapshot Backup: deletion control (part4) #54980

[Feature] Support Cluster Snapshot Backup: deletion control (part4) #54980

Conversation

srlch commented Jan 13, 2025 • edited Loading

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

starrocks-cr bot Jan 13, 2025

Choose a reason for hiding this comment

starrocks-cr bot Jan 13, 2025

Choose a reason for hiding this comment

xiangguangyxg Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 17, 2025

Quality Gate passed

github-actions bot commented Jan 17, 2025

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Jan 17, 2025

[FE Incremental Coverage Report]

file detail

github-actions bot commented Jan 17, 2025

[BE Incremental Coverage Report]

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srlch commented Jan 17, 2025

mergify bot commented Jan 17, 2025 • edited Loading

✅ Backports have been created

srlch commented Jan 13, 2025 •

edited

Loading

xiangguangyxg Jan 13, 2025 •

edited

Loading

mergify bot commented Jan 17, 2025 •

edited

Loading