diegomrsantos commented on PR #4240: URL: https://github.com/apache/iggy/pull/4240#issuecomment-5752425115
Suggested review path: start with the two controls, then follow the shared production code they exercise. This PR connects purge completion and bookmark recovery to the existing `DurableStorage` trait, which `DiskStorage` and `SimStorage` already implemented. It prepares the testing foundation for #4239 while preserving the directory sync failure behavior in #4128. A consumer offset is a bookmark: a saved offset of `2` means `Next` starts at message `3`. Purge must clear those bookmarks so consumers can read replacement messages from the beginning. The [two test scenarios](https://github.com/apache/iggy/blob/b39042a4dc1685ad0392ca054289b3582d8fdafe/core/simulator/src/storage/purge.rs#L341) make this observable: | Scenario after power loss | Bookmark loaded into the new partition | Messages returned by `Next` | | --- | --- | --- | | No purge happened | `2` | `3, 4` | | Purge completed before fresh messages arrived | None | `0, 1, 2, 3, 4` | Both scenarios cover individual consumers and groups under `Replicated` and `Persisted` offset policies. In the first test, the fixture writes and explicitly syncs the bookmarks and messages before the crash. No data loss is expected. “Recovery” means creating a new partition and loading its state from the surviving files. The simulated crash discards unsynced changes, preventing recovery from relying on cached writes. This control also rejects a recovery implementation that always returns empty bookmark maps. Because the fixture explicitly syncs under both policies, it does not claim that every ordinary `Replicated` write survives power loss. In the second test, the partition starts at the purge completion phase, with message history already reset and old bookmarks still present. It also creates bookmark files absent from the live maps, requiring the directory sweep to find them. Assertions check that completion clears the live bookmarks, durability tracking, and files. After saving fresh messages and simulating power loss, the test reloads the completion marker and consumer state into a new partition, then checks the actual messages returned by `Next`. Reusing offsets `0` through `4` is intentional: stale bookmark `2` would still be within range but would silently skip the first three messages. For the production changes, the useful review points are: 1. [Purge completion](https://github.com/apache/iggy/blob/b39042a4dc1685ad0392ca054289b3582d8fdafe/core/partitions/src/iggy_partition.rs#L7841): production passes `DiskStorage`; the harness passes `SimStorage` to the same cleanup and completion method. Check that the extraction preserves ordering and error handling, especially continuing after offset deletion or directory sync failures. The caller must already have reset message history and excluded concurrent writes. 2. [Offset persistence](https://github.com/apache/iggy/blob/b39042a4dc1685ad0392ca054289b3582d8fdafe/core/partitions/src/offset_storage.rs#L18): the existing disk entry points delegate to helpers accepting storage. Check record formats, replacement ordering, and who owns directory sync. File sync preserves contents; directory sync preserves changes to names. Syncing the purge marker's parent does not sync the separate consumer and group directories. 3. [Consumer recovery](https://github.com/apache/iggy/blob/b39042a4dc1685ad0392ca054289b3582d8fdafe/core/server/src/partition_helpers.rs#L172): the harness uses the server's loader and map initialization rather than retaining old maps. Check that validation, clamping, and persistence tracking still follow the existing boot behavior. The [directory scan adapter](https://github.com/apache/iggy/blob/b39042a4dc1685ad0392ca054289b3582d8fdafe/core/journal/src/durable_storage.rs#L297) preserves the disk scan's filtering and bounded worker behavior. 4. [Fresh message persistence](https://github.com/apache/iggy/blob/b39042a4dc1685ad0392ca054289b3582d8fdafe/core/simulator/src/storage/purge.rs#L151): it syncs the message journal and its parent, leaving the offset directories untouched. A global simulated writeback here could accidentally make a failed bookmark deletion durable and conceal the intended future failure case. These are controls for successful cleanup and recovery. They exercise production bookmark logic against simulated storage, but do not inject sync failures, exercise the message reset phase or purge retries, or run full server startup. The known bug can therefore remain while both tests pass. The remaining regression work must introduce the relevant directory sync failure and check recovery and retries, including preservation of acknowledged fresh messages. -- 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]
