Repository navigation
Clear pipe tsfile progress indexes when a data region is deleted - #19059
luoluoyuyu wants to merge 1 commit into
Conversation
PipeTsFileEpochProgressIndexKeeper keeps progress indexes per data region, but entries are only removed by pipe event callbacks. A deleted data region receives no further callbacks, so its entries stay in memory for the rest of the DataNode lifetime. Remove them in StorageEngine.deleteDataRegion and cover the cleanup with a unit test and an integration test.
Caideyipi
left a comment
There was a problem hiding this comment.
Reviewed the full diff and the relevant region-deletion, asynchronous Pipe assignment, keeper registration, and locale-selection paths at dec46bf. Two actionable issues are noted inline. The current GitHub checks show 37 successful checks and 2 skipped checks; I did not rerun Maven builds or tests during this review.
| if (region != null) { | ||
| LOGGER.info(StorageEngineMessages.REMOVING_DATA_REGION, regionId); | ||
| region.markDeleted(); | ||
| PipeTsFileEpochProgressIndexKeeper.getInstance().clearProgressIndex(regionId.getId()); |
There was a problem hiding this comment.
[P2] Prevent pending Pipe events from repopulating the deleted region
markDeleted() stops storage-side work, but it neither deregisters the realtime Pipe sources nor drains their assigner. An insert already published to PipeDataRegionAssigner can therefore reach PipeRealtimeDataRegionHybridSource.extractTabletInsertion() after this clear and call registerProgressIndex() for the deleted region; that method recreates the region entry with computeIfAbsent. In particular, a downgraded tablet from a working TsFile can reintroduce the resource after deletion, while forceCloseAllWorkingTsFileProcessors() discards that processor without publishing its normal TsFile-close event. The entry then still depends on the later source close() for removal, so the new deletion cleanup does not guarantee that the deleted region's state stays cleared. Please coordinate the final clear with outstanding registrations, or reject registrations from the obsolete region lifecycle, and cover an event paused before registration while deleteDataRegion() completes. The added UTs only register before deletion, and the IT creates no Pipe, so none exercises this ordering.
|
|
||
| private static final String DATABASE = "root.pipe_progress_index"; | ||
| private static final String CLEANUP_LOG = | ||
| "Cleared pipe progress indexes of the deleted data region"; |
There was a problem hiding this comment.
[P2] Make the cleanup assertion work with the Chinese locale
With -P with-zh-locale, the DataNode emits the Chinese StorageEngineMessages.LOG_CLEARED_PIPE_PROGRESS_INDEXES_OF_THE_DELETED_DATA_REGION_ARG_DC6E6D9F value added in this PR. This hardcoded English substring can never match that log, so running this IT with both with-integration-tests and with-zh-locale always times out after 60 seconds even when cleanup succeeds. Derive the expected message from the selected StorageEngineMessages constant (accounting for the region-id placeholder), or use an assertion independent of the log language. Compiling both locales alone does not exercise this failure.
Description
Problem
PipeTsFileEpochProgressIndexKeeperkeeps pipe tsFile dedup state per data region (regionId -> taskScope -> tsFilePath -> TsFileResource). Entries are only removed by pipe event callbacks and by the realtime source'sclose().StorageEngine.deleteDataRegion()removes the region, its files, WAL, snapshots and the receiver-side resource, but never removes this source-side dedup state. After a region is deleted (DROP DATABASE, region migration cleanup), its entries stay in the DataNode heap for the rest of the process lifetime: memory grows on every create/drop cycle, and a re-created region can be matched against stale progress indexes.Solution
PipeTsFileEpochProgressIndexKeeper.clearProgressIndex(int dataRegionId), which drops all task scopes of one region at once.StorageEngine.deleteDataRegion()right afterregion.markDeleted(), i.e. before the region files are removed, so the cleanup also happens when the rest of the deletion fails. The cleanup is logged through a newStorageEngineMessagesconstant (en and zh).Tests
PipeTsFileEpochProgressIndexKeeperTest#testClearProgressIndexRemovesAllTaskScopesOfDataRegion: entries of two different task scopes of the same region are all removed.DataRegionTest#testDeletedRegionClearsItsPipeProgressIndexes: registers an index, runs the realStorageEngine.deleteDataRegion(), asserts the entry is gone.IoTDBPipeProgressIndexCleanupIT(LocalStandaloneIT, 1C1D,SimpleIT): creates a database, writes and flushes data, drops the database, and waits for the DataNode log to report the cleanup. It fails on the pre-fix build (timeout, no cleanup record) and passes with the fix (StorageEngine:887 - Cleared pipe progress indexes of the deleted data region 1).Verified locally: keeper UT 5/5,
DataRegionTestmethod 1/1, IT 1/1,spotless:checkforiotdb-core/datanodeandintegration-test,test-compilefor the default locale and for-P with-zh-locale.This PR has:
Key changed/added classes (or packages if there are too many classes) in this PR
org.apache.iotdb.db.pipe.source.dataregion.realtime.assigner.PipeTsFileEpochProgressIndexKeeperorg.apache.iotdb.db.storageengine.StorageEngineorg.apache.iotdb.db.i18n.StorageEngineMessages(en / zh)org.apache.iotdb.db.pipe.source.dataregion.realtime.assigner.PipeTsFileEpochProgressIndexKeeperTestorg.apache.iotdb.db.storageengine.dataregion.DataRegionTestorg.apache.iotdb.db.it.IoTDBPipeProgressIndexCleanupIT