[kv] when table(partition) is deleted, drop corresponding snapshots #427
Conversation
| @@ -96,6 +97,14 @@ public CompletedSnapshotStore getOrCreateCompletedSnapshotStore(TableBucket tabl | |||
| }); | |||
| } | |||
|
|
|||
| public void onRemoveCompletedSnapshotStoreByTableBuckets(Set<TableBucket> tableBuckets) { | |||
There was a problem hiding this comment.
nit:
| public void onRemoveCompletedSnapshotStoreByTableBuckets(Set<TableBucket> tableBuckets) { | |
| public void removeCompletedSnapshotStoreByTableBuckets(Set<TableBucket> tableBuckets) { |
to make the naming style align with createCompletedSnapshotStore
| @@ -96,6 +97,14 @@ public CompletedSnapshotStore getOrCreateCompletedSnapshotStore(TableBucket tabl | |||
| }); | |||
| } | |||
|
|
|||
| public void onRemoveCompletedSnapshotStoreByTableBuckets(Set<TableBucket> tableBuckets) { | |||
| // Remove CompletedSnapshotStore is all we need to do, as remote KV files have already | |||
There was a problem hiding this comment.
I think we can remove theres comments since it may not precise for the root directory of table snapshot will be drop in coordinator server.
| @@ -499,6 +499,12 @@ private void processCreatePartition(CreatePartitionEvent createPartitionEvent) { | |||
| } | |||
|
|
|||
| private void processDropTable(DropTableEvent dropTableEvent) { | |||
| // When drop a table, drop its snapshot store meanwhile. | |||
There was a problem hiding this comment.
Only when the table is a primary key table, do we need to remove snapshot store
// When drop a primary key table, drop its snapshot store meanwhile.
if (coordinatorContext.getTableInfo(dropTableEvent.getTableId()).hasPrimaryKey()) {
Set<TableBucket> deleteTableBuckets =
coordinatorContext.getAllBucketsForTable(dropTableEvent.getTableId());
completedSnapshotStoreManager.onRemoveCompletedSnapshotStoreByTableBuckets(
deleteTableBuckets);
}
You'll need to add a method getTableInfo in CoordinatorContext
| @@ -510,6 +516,14 @@ private void processDropPartition(DropPartitionEvent dropPartitionEvent) { | |||
| TablePartition tablePartition = | |||
| new TablePartition( | |||
| dropPartitionEvent.getTableId(), dropPartitionEvent.getPartitionId()); | |||
|
|
|||
| // When drop a table partition, drop its snapshot store meanwhile. | |||
There was a problem hiding this comment.
dito:
Only when the table is a primary key table, do we need to remove snapshot store
|
@luoyuxia thanks a lot for your opinion! I've tried to fix. PTAL~ |
| @@ -149,6 +149,11 @@ public boolean hasPrimaryKey() { | |||
| return !primaryKeys.isEmpty(); | |||
| } | |||
|
|
|||
| /** Check if the table is a primary key table or a log table. */ | |||
| public boolean isPrimaryKeyTable() { | |||
There was a problem hiding this comment.
if hasPrimaryKey return true, it means it's a primaryKeyTable. We don't need to introduce this method.
| coordinatorContext.getAllBucketsForPartition(tableId, partition2Id); | ||
| int sizeofCompletedSnapshotStore4partition1 = 0; | ||
| int sizeofCompletedSnapshotStore4partition2 = 0; | ||
| for (TableBucket tableBucket : tableBuckets4partition1) { |
There was a problem hiding this comment.
nit:
We can just mock completedSnapshotStore for the partition1 which is to be drop to simlify the test
|
@luoyuxia I've simplify the code. Thanks for code review! |
Purpose
Linked issue: close #412
Currently, when kv table is deleted or table partition is expired, memory still persist its kv snapshot, which causes memory leak. This pr will fix it.
Tests
CompletedSnapshotStoreManagerTest::testRemoveCompletedSnapshotStoreFromManager
API and Format
Documentation