From 3ebfbf334253556e553eac09e4f0376b8a22a6b4 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Sep 2016 18:36:50 +0200 Subject: [PATCH 1/2] fix migration system, better errors --- parity/migration.rs | 11 +++++++---- util/src/migration/mod.rs | 27 ++++++++++++++++++--------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/parity/migration.rs b/parity/migration.rs index ac96d0864..0562f388b 100644 --- a/parity/migration.rs +++ b/parity/migration.rs @@ -43,13 +43,15 @@ pub enum Error { /// Returned when current version cannot be read or guessed. UnknownDatabaseVersion, /// Migration does not support existing pruning algorithm. - UnsuportedPruningMethod, + UnsupportedPruningMethod, /// Existing DB is newer than the known one. FutureDBVersion, /// Migration is not possible. MigrationImpossible, /// Migration unexpectadly failed. MigrationFailed, + /// Internal migration error. + Internal(MigrationError), /// Migration was completed succesfully, /// but there was a problem with io. Io(IoError), @@ -59,10 +61,11 @@ impl Display for Error { fn fmt(&self, f: &mut Formatter) -> Result<(), FmtError> { let out = match *self { Error::UnknownDatabaseVersion => "Current database version cannot be read".into(), - Error::UnsuportedPruningMethod => "Unsupported pruning method for database migration. Delete DB and resync.".into(), + Error::UnsupportedPruningMethod => "Unsupported pruning method for database migration. Delete DB and resync.".into(), Error::FutureDBVersion => "Database was created with newer client version. Upgrade your client or delete DB and resync.".into(), Error::MigrationImpossible => format!("Database migration to version {} is not possible.", CURRENT_VERSION), Error::MigrationFailed => "Database migration unexpectedly failed".into(), + Error::Internal(ref err) => format!("{}", err), Error::Io(ref err) => format!("Unexpected io error on DB migration: {}.", err), }; @@ -80,7 +83,7 @@ impl From for Error { fn from(err: MigrationError) -> Self { match err { MigrationError::Io(e) => Error::Io(e), - _ => Error::MigrationFailed, + _ => Error::Internal(err), } } } @@ -320,7 +323,7 @@ mod legacy { let res = match pruning { Algorithm::Archive => manager.add_migration(migrations::state::ArchiveV7::default()), Algorithm::OverlayRecent => manager.add_migration(migrations::state::OverlayRecentV7::default()), - _ => return Err(Error::UnsuportedPruningMethod), + _ => return Err(Error::UnsupportedPruningMethod), }; try!(res.map_err(|_| Error::MigrationImpossible)); diff --git a/util/src/migration/mod.rs b/util/src/migration/mod.rs index cfd828086..176283668 100644 --- a/util/src/migration/mod.rs +++ b/util/src/migration/mod.rs @@ -20,6 +20,7 @@ mod tests; use std::collections::BTreeMap; use std::fs; +use std::fmt; use std::path::{Path, PathBuf}; use ::kvdb::{CompactionProfile, Database, DatabaseConfig, DBTransaction}; @@ -96,6 +97,17 @@ pub enum Error { Custom(String), } +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { + match *self { + Error::CannotAddMigration => write!(f, "Cannot add migration"), + Error::MigrationImpossible => write!(f, "Migration impossible"), + Error::Io(ref err) => write!(f, "{}", err), + Error::Custom(ref err) => write!(f, "{}", err), + } + } +} + impl From<::std::io::Error> for Error { fn from(e: ::std::io::Error) -> Self { Error::Io(e) @@ -104,6 +116,8 @@ impl From<::std::io::Error> for Error { /// A generalized migration from the given db to a destination db. pub trait Migration: 'static { + /// Number of columns in the database before the migration. + fn pre_columns(&self) -> Option { self.columns() } /// Number of columns in database after the migration. fn columns(&self) -> Option; /// Version of the database after the migration. @@ -195,6 +209,7 @@ impl Manager { Some(last) => migration.version() > last.version(), None => true, }; + match is_new { true => Ok(self.migrations.push(Box::new(migration))), false => Err(Error::CannotAddMigration), @@ -205,9 +220,11 @@ impl Manager { /// and producing a path where the final migration lives. pub fn execute(&mut self, old_path: &Path, version: u32) -> Result { let config = self.config.clone(); - let columns = self.no_of_columns_at(version); let migrations = self.migrations_from(version); if migrations.is_empty() { return Err(Error::MigrationImpossible) }; + + let columns = migrations.iter().find(|m| m.version() == version).and_then(|m| m.pre_columns()); + let mut db_config = DatabaseConfig { max_open_files: 64, cache_size: None, @@ -267,14 +284,6 @@ impl Manager { fn migrations_from(&mut self, version: u32) -> Vec<&mut Box> { self.migrations.iter_mut().filter(|m| m.version() > version).collect() } - - fn no_of_columns_at(&self, version: u32) -> Option { - let migration = self.migrations.iter().find(|m| m.version() == version); - match migration { - Some(m) => m.columns(), - None => None - } - } } /// Prints a dot every `max` ticks From b8b3f066c4546bc1c0d2186db09dd249b98ed599 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 26 Sep 2016 18:24:58 +0200 Subject: [PATCH 2/2] add a test --- util/src/migration/tests.rs | 45 +++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/util/src/migration/tests.rs b/util/src/migration/tests.rs index ee5ff574e..05229bee5 100644 --- a/util/src/migration/tests.rs +++ b/util/src/migration/tests.rs @@ -19,7 +19,7 @@ //! are performed in temp sub-directories. use common::*; -use migration::{Config, SimpleMigration, Manager}; +use migration::{Batch, Config, Error, SimpleMigration, Migration, Manager}; use kvdb::Database; use devtools::RandomTempPath; @@ -62,11 +62,10 @@ impl SimpleMigration for Migration0 { fn version(&self) -> u32 { 1 } - fn simple_migrate(&mut self, key: Vec, value: Vec) -> Option<(Vec, Vec)> { - let mut key = key; + fn simple_migrate(&mut self, mut key: Vec, mut value: Vec) -> Option<(Vec, Vec)> { key.push(0x11); - let mut value = value; value.push(0x22); + Some((key, value)) } } @@ -83,6 +82,31 @@ impl SimpleMigration for Migration1 { } } +struct AddsColumn; + +impl Migration for AddsColumn { + fn pre_columns(&self) -> Option { None } + + fn columns(&self) -> Option { Some(1) } + + fn version(&self) -> u32 { 1 } + + fn migrate(&mut self, source: &Database, config: &Config, dest: &mut Database, col: Option) -> Result<(), Error> { + let mut batch = Batch::new(config, col); + + for (key, value) in source.iter(col) { + try!(batch.insert(key.to_vec(), value.to_vec(), dest)); + } + + + if col == Some(1) { + try!(batch.insert(vec![1, 2, 3], vec![4, 5, 6], dest)); + } + + batch.commit(dest) + } +} + #[test] fn one_simple_migration() { let dir = RandomTempPath::create_dir(); @@ -189,3 +213,16 @@ fn is_migration_needed() { assert!(manager.is_needed(1)); assert!(!manager.is_needed(2)); } + +#[test] +fn pre_columns() { + let mut manager = Manager::new(Config::default()); + manager.add_migration(AddsColumn).unwrap(); + + let dir = RandomTempPath::create_dir(); + let db_path = db_path(dir.as_path()); + + // this shouldn't fail to open the database even though it's one column + // short of the one before it. + manager.execute(&db_path, 0).unwrap(); +}