Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

JournalDB inject #1806

Merged
merged 4 commits into from
Aug 3, 2016
Merged

JournalDB inject #1806

merged 4 commits into from
Aug 3, 2016

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Aug 2, 2016

More generic approach to assist snapshot restoration.

@rphmeier rphmeier added the A0-pleasereview 🤓 Pull request needs code review. label Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.03%) to 87.014% when pulling ec66960 on journaldb_inject into 21c65a9 on master.

let (key, (value, rc)) = i;
if rc > 0 {
assert!(rc == 1);
batch.put(self.column, &key, &value).expect("Low-level database error. Some issue with your hard disk?");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sending this out through UtilError?

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.02%) to 87.001% when pulling 8aaf820 on journaldb_inject into 21c65a9 on master.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 3, 2016
}
if rc < 0 {
assert!(rc == -1);
if try!(self.backing.get(self.column, &key)).is_none() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these checks probably will slow down inject quite a bit and it's not as though these are going to cause rocksdb errors. are they even worthwhile?

seems like we are using the DB to point out logic errors in the rest of our code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup - this way it fails fast rather than allows an inconsistent situation to persevere.

however, we might consider a build flag (perhaps called "final"?) which allows these checks to be omitted.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 3, 2016
@gavofyork gavofyork merged commit 8c88e2a into master Aug 3, 2016
@gavofyork gavofyork deleted the journaldb_inject branch August 3, 2016 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants