Skip to content

Commit 9974eed

Browse files
mergify[bot]gengjun-git
mergify[bot]
and
gengjun-git
authored
[Enhancement] Optimize CatalogRecycleBin adjusted recycle timestamp lookup (backport #72128) (#72138)
Signed-off-by: gengjun-git <gengjun@starrocks.com> Co-authored-by: gengjun-git <gengjun@starrocks.com>
1 parent 1fb048a commit 9974eed

3 files changed

Lines changed: 47 additions & 37 deletions

File tree

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

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -307,15 +307,8 @@ private synchronized boolean checkValidDeletionByClusterSnapshot(long id) {
307307
return GlobalStateMgr.getCurrentState().getClusterSnapshotMgr().isDeletionSafeToExecute(originalRecycleTime);
308308
}
309309

310-
private synchronized long getAdjustedRecycleTimestamp(long id) {
311-
Map<Long, RecycleTableInfo> idToRecycleTableInfo = Maps.newHashMap();
312-
for (Map<Long, RecycleTableInfo> tableEntry : idToTableInfo.rowMap().values()) {
313-
for (Map.Entry<Long, RecycleTableInfo> entry : tableEntry.entrySet()) {
314-
idToRecycleTableInfo.put(entry.getKey(), entry.getValue());
315-
}
316-
}
317-
318-
RecycleTableInfo tableInfo = idToRecycleTableInfo.get(id);
310+
private synchronized long getAdjustedRecycleTimestamp(long dbId, long id) {
311+
RecycleTableInfo tableInfo = idToTableInfo.row(dbId).get(id);
319312
if (tableInfo != null && !tableInfo.isRecoverable()) {
320313
return 0;
321314
}
@@ -342,8 +335,8 @@ private synchronized long getAdjustedRecycleTimestamp(long id) {
342335
* if we can erase this instance, we should check if anyone enable erase later.
343336
* Only used by main loop.
344337
*/
345-
private synchronized boolean timeExpired(long id, long currentTimeMs) {
346-
long latencyMs = currentTimeMs - getAdjustedRecycleTimestamp(id);
338+
private synchronized boolean timeExpired(long dbId, long id, long currentTimeMs) {
339+
long latencyMs = currentTimeMs - getAdjustedRecycleTimestamp(dbId, id);
347340
long expireMs = max(Config.catalog_trash_expire_second * 1000L, Config.catalog_recycle_bin_erase_min_latency_ms);
348341
// customize expireMs for partition that need to be retained for a configurable period
349342
if (idToPartition.containsKey(id)) {
@@ -371,7 +364,7 @@ private synchronized boolean canEraseDatabase(RecycleDatabaseInfo databaseInfo,
371364
return false;
372365
}
373366

374-
if (timeExpired(databaseInfo.getDb().getId(), currentTimeMs)) {
367+
if (timeExpired(databaseInfo.getDb().getId(), databaseInfo.getDb().getId(), currentTimeMs)) {
375368
return true;
376369
}
377370

@@ -388,7 +381,7 @@ private synchronized boolean canEraseTable(RecycleTableInfo tableInfo, long curr
388381
return false;
389382
}
390383

391-
if (timeExpired(tableInfo.getTable().getId(), currentTimeMs)) {
384+
if (timeExpired(tableInfo.getDbId(), tableInfo.getTable().getId(), currentTimeMs)) {
392385
return true;
393386
}
394387

@@ -410,7 +403,7 @@ private synchronized boolean canErasePartition(RecyclePartitionInfo partitionInf
410403
return false;
411404
}
412405

413-
if (timeExpired(partitionInfo.getPartition().getId(), currentTimeMs)) {
406+
if (timeExpired(partitionInfo.getDbId(), partitionInfo.getPartition().getId(), currentTimeMs)) {
414407
return true;
415408
}
416409

@@ -432,13 +425,13 @@ private synchronized boolean canErasePartition(RecyclePartitionInfo partitionInf
432425
/**
433426
* make sure there are still some time before the subject is erased
434427
*/
435-
public synchronized boolean ensureEraseLater(long id, long currentTimeMs) {
428+
public synchronized boolean ensureEraseLater(long dbId, long id, long currentTimeMs) {
436429
// 1. not in idToRecycleTime, maybe already erased, sorry it's too late!
437430
if (!idToRecycleTime.containsKey(id)) {
438431
return false;
439432
}
440433
// 2. will expire after quite a long time, don't worry
441-
long latency = currentTimeMs - getAdjustedRecycleTimestamp(id);
434+
long latency = currentTimeMs - getAdjustedRecycleTimestamp(dbId, id);
442435
if (latency < (Config.catalog_trash_expire_second - LATE_RECYCLE_INTERVAL_SECONDS) * 1000L) {
443436
return true;
444437
}

‎fe/fe-core/src/main/java/com/starrocks/clone/TabletScheduler.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -562,19 +562,19 @@ protected boolean checkIfTabletExpired(TabletSchedCtx ctx) {
562562
protected boolean checkIfTabletExpired(TabletSchedCtx ctx, CatalogRecycleBin recycleBin, long currentTimeMs) {
563563
// check if about to erase
564564
long dbId = ctx.getDbId();
565-
if (recycleBin.getDatabase(dbId) != null && !recycleBin.ensureEraseLater(dbId, currentTimeMs)) {
565+
if (recycleBin.getDatabase(dbId) != null && !recycleBin.ensureEraseLater(dbId, dbId, currentTimeMs)) {
566566
LOG.warn("discard ctx because db {} will erase soon: {}", dbId, ctx);
567567
return true;
568568
}
569569
long tableId = ctx.getTblId();
570-
if (recycleBin.getTable(dbId, tableId) != null && !recycleBin.ensureEraseLater(tableId, currentTimeMs)) {
570+
if (recycleBin.getTable(dbId, tableId) != null && !recycleBin.ensureEraseLater(dbId, tableId, currentTimeMs)) {
571571
LOG.warn("discard ctx because table {} will erase soon: {}", tableId, ctx);
572572
return true;
573573
}
574574
long partitionId = ctx.getPhysicalPartitionId();
575575
PhysicalPartition physicalPartition = recycleBin.getPhysicalPartition(partitionId);
576576
if (physicalPartition != null
577-
&& !recycleBin.ensureEraseLater(physicalPartition.getParentId(), currentTimeMs)) {
577+
&& !recycleBin.ensureEraseLater(dbId, physicalPartition.getParentId(), currentTimeMs)) {
578578
LOG.warn("discard ctx because partition {} will erase soon: {}", partitionId, ctx);
579579
return true;
580580
}

‎fe/fe-core/src/test/java/com/starrocks/catalog/CatalogRecycleBinTest.java‎

Lines changed: 35 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -411,21 +411,21 @@ public void testEnsureEraseLater() {
411411

412412
// no need to set enable erase later if there are a lot of time left
413413
long now = System.currentTimeMillis();
414-
Assertions.assertTrue(recycleBin.ensureEraseLater(db.getId(), now));
414+
Assertions.assertTrue(recycleBin.ensureEraseLater(db.getId(), db.getId(), now));
415415
Assertions.assertFalse(recycleBin.enableEraseLater.contains(db.getId()));
416416

417417
// no need to set enable erase later if already exipre
418418
long moreThanTenMinutesLater = now + 620 * 1000L;
419-
Assertions.assertFalse(recycleBin.ensureEraseLater(db.getId(), moreThanTenMinutesLater));
419+
Assertions.assertFalse(recycleBin.ensureEraseLater(db.getId(), db.getId(), moreThanTenMinutesLater));
420420
Assertions.assertFalse(recycleBin.enableEraseLater.contains(db.getId()));
421421

422422
// now we should set enable erase later because we are about to expire
423423
long moreThanNineMinutesLater = now + 550 * 1000L;
424-
Assertions.assertTrue(recycleBin.ensureEraseLater(db.getId(), moreThanNineMinutesLater));
424+
Assertions.assertTrue(recycleBin.ensureEraseLater(db.getId(), db.getId(), moreThanNineMinutesLater));
425425
Assertions.assertTrue(recycleBin.enableEraseLater.contains(db.getId()));
426426

427427
// if already expired, we should return false but won't erase the flag
428-
Assertions.assertFalse(recycleBin.ensureEraseLater(db.getId(), moreThanTenMinutesLater));
428+
Assertions.assertFalse(recycleBin.ensureEraseLater(db.getId(), db.getId(), moreThanTenMinutesLater));
429429
Assertions.assertTrue(recycleBin.enableEraseLater.contains(db.getId()));
430430
}
431431

@@ -523,11 +523,11 @@ public void testRecycleDb(@Mocked GlobalStateMgr globalStateMgr, @Mocked EditLog
523523

524524
// 3. set recyle later, check if recycle now
525525
CatalogRecycleBin.LATE_RECYCLE_INTERVAL_SECONDS = 10;
526-
Assertions.assertFalse(recycleBin.ensureEraseLater(db1.getId(), now)); // already erased
527-
Assertions.assertTrue(recycleBin.ensureEraseLater(db2.getId(), now));
526+
Assertions.assertFalse(recycleBin.ensureEraseLater(db1.getId(), db1.getId(), now)); // already erased
527+
Assertions.assertTrue(recycleBin.ensureEraseLater(db2.getId(), db2.getId(), now));
528528
Assertions.assertEquals(0, recycleBin.enableEraseLater.size());
529529
recycleBin.idToRecycleTime.put(db2.getId(), expireFromNow + 1000);
530-
Assertions.assertTrue(recycleBin.ensureEraseLater(db2.getId(), now));
530+
Assertions.assertTrue(recycleBin.ensureEraseLater(db2.getId(), db2.getId(), now));
531531
Assertions.assertEquals(1, recycleBin.enableEraseLater.size());
532532
Assertions.assertTrue(recycleBin.enableEraseLater.contains(db2.getId()));
533533

@@ -539,7 +539,7 @@ public void testRecycleDb(@Mocked GlobalStateMgr globalStateMgr, @Mocked EditLog
539539

540540
// 5. will erase after expire time + latency time
541541
recycleBin.idToRecycleTime.put(db2.getId(), expireFromNow - 11000);
542-
Assertions.assertFalse(recycleBin.ensureEraseLater(db2.getId(), now));
542+
Assertions.assertFalse(recycleBin.ensureEraseLater(db2.getId(), db2.getId(), now));
543543
recycleBin.eraseDatabase(now);
544544
Assertions.assertNull(recycleBin.getDatabase(db2.getId()));
545545
Assertions.assertEquals(0, recycleBin.idToRecycleTime.size());
@@ -652,11 +652,11 @@ public void testRecycleTable(@Mocked GlobalStateMgr globalStateMgr, @Mocked Edit
652652

653653
// 3. set recyle later, check if recycle now
654654
CatalogRecycleBin.LATE_RECYCLE_INTERVAL_SECONDS = 10;
655-
Assertions.assertFalse(recycleBin.ensureEraseLater(table1.getId(), now)); // already erased
656-
Assertions.assertTrue(recycleBin.ensureEraseLater(table2.getId(), now));
655+
Assertions.assertFalse(recycleBin.ensureEraseLater(dbId, table1.getId(), now)); // already erased
656+
Assertions.assertTrue(recycleBin.ensureEraseLater(dbId, table2.getId(), now));
657657
Assertions.assertEquals(0, recycleBin.enableEraseLater.size());
658658
recycleBin.idToRecycleTime.put(table2.getId(), expireFromNow + 1000);
659-
Assertions.assertTrue(recycleBin.ensureEraseLater(table2.getId(), now));
659+
Assertions.assertTrue(recycleBin.ensureEraseLater(dbId, table2.getId(), now));
660660
Assertions.assertEquals(1, recycleBin.enableEraseLater.size());
661661
Assertions.assertTrue(recycleBin.enableEraseLater.contains(table2.getId()));
662662

@@ -669,7 +669,7 @@ public void testRecycleTable(@Mocked GlobalStateMgr globalStateMgr, @Mocked Edit
669669

670670
// 5. will erase after expire time + latency time
671671
recycleBin.idToRecycleTime.put(table2.getId(), expireFromNow - 11000);
672-
Assertions.assertFalse(recycleBin.ensureEraseLater(table2.getId(), now));
672+
Assertions.assertFalse(recycleBin.ensureEraseLater(dbId, table2.getId(), now));
673673
recycleBin.eraseTable(now);
674674
waitPartitionClearFinished(recycleBin, table2.getId(), now);
675675
Assertions.assertNull(recycleBin.getTable(dbId, table2.getId()));
@@ -744,11 +744,11 @@ public void testRecyclePartition(@Mocked GlobalStateMgr globalStateMgr, @Mocked
744744

745745
// 3. set recyle later, check if recycle now
746746
CatalogRecycleBin.LATE_RECYCLE_INTERVAL_SECONDS = 10;
747-
Assertions.assertFalse(recycleBin.ensureEraseLater(p1.getId(), now)); // already erased
748-
Assertions.assertTrue(recycleBin.ensureEraseLater(p2.getId(), now));
747+
Assertions.assertFalse(recycleBin.ensureEraseLater(dbId, p1.getId(), now)); // already erased
748+
Assertions.assertTrue(recycleBin.ensureEraseLater(dbId, p2.getId(), now));
749749
Assertions.assertEquals(0, recycleBin.enableEraseLater.size());
750750
recycleBin.idToRecycleTime.put(p2.getId(), expireFromNow + 1000);
751-
Assertions.assertTrue(recycleBin.ensureEraseLater(p2.getId(), now));
751+
Assertions.assertTrue(recycleBin.ensureEraseLater(dbId, p2.getId(), now));
752752
Assertions.assertEquals(1, recycleBin.enableEraseLater.size());
753753
Assertions.assertTrue(recycleBin.enableEraseLater.contains(p2.getId()));
754754

@@ -761,7 +761,7 @@ public void testRecyclePartition(@Mocked GlobalStateMgr globalStateMgr, @Mocked
761761

762762
// 5. will erase after expire time + latency time
763763
recycleBin.idToRecycleTime.put(p2.getId(), expireFromNow - 11000);
764-
Assertions.assertFalse(recycleBin.ensureEraseLater(p2.getId(), now));
764+
Assertions.assertFalse(recycleBin.ensureEraseLater(dbId, p2.getId(), now));
765765
recycleBin.erasePartition(now);
766766
waitPartitionClearFinished(recycleBin, p2.getId(), now);
767767
Assertions.assertEquals(recycleBin.getPartition(p2.getId()), null);
@@ -1174,14 +1174,31 @@ public void testGetAdjustedRecycleTimestampWithRetentionPeriod() {
11741174
recycleBin.recyclePartition(info2);
11751175

11761176
// With retention period: should return original recycle timestamp
1177-
long adjustedTime1 = Deencapsulation.invoke(recycleBin, "getAdjustedRecycleTimestamp", p1.getId());
1177+
long adjustedTime1 = Deencapsulation.invoke(recycleBin, "getAdjustedRecycleTimestamp", dbId, p1.getId());
11781178
Assertions.assertEquals(recycleBin.idToRecycleTime.get(p1.getId()), adjustedTime1);
11791179

11801180
// Without retention period: should return 0 for non-recoverable partition
1181-
long adjustedTime2 = Deencapsulation.invoke(recycleBin, "getAdjustedRecycleTimestamp", p2.getId());
1181+
long adjustedTime2 = Deencapsulation.invoke(recycleBin, "getAdjustedRecycleTimestamp", dbId, p2.getId());
11821182
Assertions.assertEquals(0, adjustedTime2);
11831183
}
11841184

1185+
@Test
1186+
public void testGetAdjustedRecycleTimestampUsesDbIdForTableLookup() {
1187+
CatalogRecycleBin recycleBin = new CatalogRecycleBin();
1188+
long dbId = 1L;
1189+
long otherDbId = 2L;
1190+
Table table = new Table(101L, "tbl", Table.TableType.VIEW, null);
1191+
recycleBin.recycleTable(dbId, table, false);
1192+
1193+
long adjustedTime = Deencapsulation.invoke(
1194+
recycleBin, "getAdjustedRecycleTimestamp", dbId, table.getId());
1195+
Assertions.assertEquals(0, adjustedTime);
1196+
1197+
long adjustedTimeWithOtherDb = Deencapsulation.invoke(
1198+
recycleBin, "getAdjustedRecycleTimestamp", otherDbId, table.getId());
1199+
Assertions.assertEquals(recycleBin.idToRecycleTime.get(table.getId()), adjustedTimeWithOtherDb);
1200+
}
1201+
11851202
/**
11861203
* Regression test for the bug where disableRecoverPartitionWithSameName() would unconditionally
11871204
* reset idToRecycleTime for already-non-recoverable partitions with retention periods, causing

0 commit comments

Comments
 (0)