Closed fabriceclementz closed 2 years ago
Incredible job. It will help to keep safe using Replibyte across different versions. I review it :)
For me, it's all good. My only feedback would be to maybe provide a way to properly execute version updates. Instead of hardcoding version check and schema upgrade. We can maybe provide a trait with a function that can be executed if the version is equal to or lower than a certain number until it reaches the target. (It's probably premature optimization since we don't have to deal with too many version changes right now. We can keep an eye on that)
Thx for your feedback @evoxmusic I really appreciate!
To be sure to understand what you think of to have a better implementation for the long run. Is it for example a Migration trait like so:
Example:
trait Migration {
/// minimal version for which the migration needs to be triggered.
fn minimal_version(&self) -> Version;
/// run the migration.
fn run(&self, datastore: &Box<dyn Datastore>) -> Result<(), Error>;
}
// Migrator handles the logic to run migrations, he knows which migrations needs to be run
pub struct Migrator<'a> {
datastore: &'a Box<dyn Datastore>,
migrations: Vec<Box<dyn Migration>>,
}
impl<'a> Migrator<'a> {
pub fn new(datastore: &'a Box<dyn Datastore>) -> Self {
Self {
datastore,
migrations: vec![Box::new(AddVersionNumber::default())],
}
}
/// run all registered migrations when the minimal version is matched.
pub fn migrate(&self) -> Result<(), Error> {
for migration in &self.migrations {
if self.should_run_migration(migration) {
let _ = migration.run(&self.datastore)?;
}
}
Ok(())
}
fn should_run_migration(&self, migration: &Box<dyn Migration>) -> bool {
// TODO: fix this hardcoded implementation
true
}
}
And an implementation of this Migration
trait for adding/updating the version number should look like:
pub struct AddVersionNumber {}
impl AddVersionNumber {
pub fn default() -> Self {
Self {}
}
}
impl Migration for AddVersionNumber {
fn minimal_version(&self) -> Version {
"0.7.4".into()
}
fn run(&self, datastore: &Box<dyn Datastore>) -> Result<(), Error> {
info!("migrate: add version number");
let mut raw_index_file = datastore.raw_index_file()?;
match raw_index_file.as_object_mut() {
Some(metadata) => {
// TODO: fix to add insert or update
metadata.insert("v".to_string(), json!(get_replibyte_version()));
}
None => {
return Err(Error::new(
ErrorKind::Other,
"migrate: metadata.json is not an object",
))
}
}
datastore.write_raw_index_file(&raw_index_file)
}
}
The migrator would be executed after the datastore is instantiate:
let migrator = Migrator::new(&datastore);
let _ = migrator.migrate()?;
Exactly π― - this is exactly what I had in mind ππ½ (sorry for letting you interpret it π )
Exactly π― - this is exactly what I had in mind ππ½ (sorry for letting you interpret it π )
No problem! I think it's ok now, I've refactored a to add a trait. As you said, It's probably a premature optimization but it's ready if we need to add more migrations π The metadata.json updates are running smoothly with both datastore.
Hi @evoxmusic This PR adds a
release.sh
script to handle version number change in the differentcargo.toml
sub crates. He's strongly inspired from https://github.com/tremor-rs/tremor-runtime/blob/main/release.sh (thx to them).I've added a version number in the
metadata.json
file which correspond to the current RepliByte version and rename thebackups
key todumps
. As there is no version number now, I trigger a "migration" to handle that without breaking changes.I need to verify some things but you can already review it. Thx for your feedback. Note: once, you upgrade with a newer replibyte version, you can't rollback without manually editing the metadata.json file as is updated.
Closes #108