Feature/migrate modification history #2260

acerone85 · 2024-09-26T21:49:03Z

Linked Issues/PRs

Closes #2095

Description

Builds on top of Versioned Storage for Modifications History #2233, which defines the migration functions for the modifications history
Defines rocksdb specific handles for the combined database (available only when the feature "rocksdb" is specified), which are necessary to access the functions for performing the migration
Spins 4 blocking tasks, one for each database, to perform the migration in parallel with the startup of the Fuel service

Questions:

Do we want to handle graceful shutdown of the migration service?
How do we test this over a large amount of data?

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

CHANGELOG.md

bin/fuel-core/src/cli/run.rs

crates/fuel-core/src/state/historical_rocksdb.rs

rafal-ch · 2024-09-27T14:29:07Z

bin/fuel-core/src/cli/run.rs

+                while let Err(e) =
+                    rocks_db.migrate_modifications_history_at_height(height)
+                {
+                    tracing::warn!("Could not migrate modifications history at height {height}: {e:?}. Trying again.")
+                }
+            }
+        }
+        rocks_db.complete_migration();


In case of Err we will just warn in log and continue with next entry, and then call complete_migration();.

I'm wondering if this leaves some entries not migrated? And if this is acceptable outcome.

I may be lacking some context, though.

That's a good question. I also lack context to answer, probably @xgreenx knows

Ah, I missed the point of your question. The rationale behind the loop is because I don't know whether we use pessmiistic or optimistic concurrency control, e.g. whether the migration transaction aborts in case of conflict. In the latter case, we need to retry the transaction. But if we employ pessimistic concurrency control then the loop is not necessary

I think we need to skip failed entry and continue. But with changes that I asked from #2233 (comment), you will not have cases where error can occur.

crates/fuel-core/src/state/historical_rocksdb.rs

xgreenx · 2024-10-01T08:30:41Z

bin/fuel-core/src/cli/run.rs

+    // TODO: Handle graceful shutdown of migration processes
+    #[cfg(feature = "rocksdb")]
+    {
+        let on_chain_migration_handle = combined_database


I think the HistoricalRocksDB should be responsible for launching the migration on construction.

xgreenx · 2024-10-01T08:33:01Z

crates/fuel-core/src/state/historical_rocksdb.rs

@@ -329,10 +352,14 @@ where
    fn rollback_block_to(&self, height_to_rollback: u64) -> StorageResult<()> {
        let mut storage_transaction = self.db.read_transaction();

+        let modifications_history_migration_in_progress = self


Looks like would be nice to have a getter for this field=)

xgreenx · 2024-10-01T08:36:19Z

bin/fuel-core/src/cli/run.rs

+                while let Err(e) =
+                    rocks_db.migrate_modifications_history_at_height(height)
+                {
+                    tracing::warn!("Could not migrate modifications history at height {height}: {e:?}. Trying again.")
+                }
+            }
+        }
+        rocks_db.complete_migration();


I think we need to skip failed entry and continue. But with changes that I asked from #2233 (comment), you will not have cases where error can occur.

rafal-ch · 2024-10-29T13:15:38Z

crates/fuel-core/src/state/historical_rocksdb.rs

+            ConflictPolicy::Overwrite,
+            Changes::default(),
+        );
+


Maybe we can add assert here? It's not crucial, but helps a bit with the clarity of the flow.

let v1_changes = historical_rocks_db .db .iter_all::<ModificationsHistoryV1<OnChain>>(None) .count(); let v2_changes = historical_rocks_db .db .iter_all::<ModificationsHistoryV2<OnChain>>(None) .count(); assert_eq!(v1_changes, 0); assert_eq!(v2_changes, 100);

rafal-ch · 2024-10-29T13:20:00Z

crates/fuel-core/src/state/historical_rocksdb.rs

+                .rollback_last_block()
+            {
+                // If rolling back fails, then cumulative changes are not being committed to rocksDB.
+                // We flush them the DB to keep the migration going.


Nit:
Looks like an incomplete comment.

rafal-ch · 2024-10-29T13:28:38Z

crates/fuel-core/src/state/historical_rocksdb.rs

+        historical_rocksdb: Arc<InnerHistoricalRocksDB<Description>>,
+    ) {
+        historical_rocksdb.is_migration_in_progress().then(|| {
+            tokio::task::spawn_blocking(move || {


Is there currently a way to gracefully interrupt this migration, if need be?

acerone85 requested review from xgreenx, Dentosal and MitchTurner as code owners September 26, 2024 21:49

acerone85 requested a review from a team September 26, 2024 21:49

acerone85 mentioned this pull request Sep 26, 2024

Versioned Storage for Modifications History #2233

Open

13 tasks

rafal-ch reviewed Sep 27, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

rafal-ch reviewed Sep 27, 2024

View reviewed changes

acerone85 force-pushed the feature/migrate-modification-history branch from a29e3a8 to e671b0c Compare September 30, 2024 13:44

acerone85 force-pushed the feature/version-modifications-history branch from 910186c to fe3e453 Compare September 30, 2024 15:55

acerone85 force-pushed the feature/migrate-modification-history branch 2 times, most recently from 5c70a37 to a3ecd66 Compare September 30, 2024 17:01

xgreenx reviewed Oct 1, 2024

View reviewed changes

acerone85 force-pushed the feature/version-modifications-history branch from 8d4bd59 to 9e6247a Compare October 1, 2024 17:45

acerone85 force-pushed the feature/migrate-modification-history branch 2 times, most recently from 943f60c to 0ff920e Compare October 1, 2024 19:01

acerone85 force-pushed the feature/version-modifications-history branch from 9e6247a to f787cf6 Compare October 2, 2024 14:04

acerone85 force-pushed the feature/migrate-modification-history branch 2 times, most recently from 8ac1bf0 to db2ffc5 Compare October 2, 2024 15:50

acerone85 force-pushed the feature/version-modifications-history branch from f787cf6 to 308c34a Compare October 3, 2024 15:57

acerone85 force-pushed the feature/migrate-modification-history branch from 7c1e835 to e1bc889 Compare October 3, 2024 15:57

acerone85 force-pushed the feature/version-modifications-history branch from 308c34a to 3c9eff4 Compare October 3, 2024 16:45

acerone85 force-pushed the feature/migrate-modification-history branch 4 times, most recently from a3f9c25 to 4e7e614 Compare October 10, 2024 15:35

acerone85 self-assigned this Oct 14, 2024

rafal-ch reviewed Oct 29, 2024

View reviewed changes

AurelienFT approved these changes Oct 29, 2024

View reviewed changes

Define migration state and prepare HistoricalDB to handle migration

a73b71b

acerone85 added 5 commits October 31, 2024 15:36

Do not migrate stale data

62bacf0

Refactor revert stale migration changes

c1906b8

Enable HistoricalRocksDB to add migration changes

69e38f1

Remove old migration in progress function

3d88ac5

Migration process

dbc1786

acerone85 force-pushed the feature/migrate-modification-history branch from db51eeb to dbc1786 Compare October 31, 2024 15:37

Improvements to migration state

f519de4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/migrate modification history #2260

Feature/migrate modification history #2260

acerone85 commented Sep 26, 2024

rafal-ch Sep 27, 2024

acerone85 Sep 30, 2024

acerone85 Sep 30, 2024 •

edited

Loading

xgreenx Oct 1, 2024

xgreenx Oct 1, 2024

xgreenx Oct 1, 2024

xgreenx Oct 1, 2024

rafal-ch Oct 29, 2024

rafal-ch Oct 29, 2024

rafal-ch Oct 29, 2024

Feature/migrate modification history #2260

Are you sure you want to change the base?

Feature/migrate modification history #2260

Conversation

acerone85 commented Sep 26, 2024

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 Sep 30, 2024 • 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

acerone85 Sep 30, 2024 •

edited

Loading