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

db: remove wal disabling / fast-and-loose option. #8963

Merged
merged 15 commits into from
Jul 10, 2018
Merged

db: remove wal disabling / fast-and-loose option. #8963

merged 15 commits into from
Jul 10, 2018

Conversation

5chdn
Copy link
Contributor

@5chdn 5chdn commented Jun 24, 2018 •
edited
Loading

fix #8779

ref #1765

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M2-config 📂 Chain specifications and node configurations. labels Jun 24, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 24, 2018
@5chdn 5chdn mentioned this pull request Jun 24, 2018
Closed
dvdplm
dvdplm previously approved these changes Jun 25, 2018
View reviewed changes
@andresilva
Copy link
Contributor

I'd actually say we remove it since this is really unsafe and will just give us trouble (more issues with database corruption). It's better to just provide a bigger cache size for performance.

niklasad1
niklasad1 previously approved these changes Jun 28, 2018
View reviewed changes
@niklasad1 niklasad1 dismissed their stale review June 28, 2018 12:01

Missed Andre's concern!

@5chdn 5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Jun 28, 2018
@5chdn
Copy link
Contributor Author

5chdn commented Jun 28, 2018

Hold on.

@5chdn 5chdn changed the title parity: highlight --fast-and-loose is not recommended. parity: remove fast-and-loose option. Jul 2, 2018
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 2, 2018
@5chdn 5chdn requested a review from niklasad1 July 2, 2018 13:29
@5chdn 5chdn requested a review from dvdplm July 2, 2018 13:34
andresilva
andresilva suggested changes Jul 2, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

I think we can remove the wal from all types that currently have it and also the handling for it in kvdb-rocksdb. A grep for wal: bool should help :)

parity/cli/mod.rs Outdated
@@ -883,6 +879,10 @@ usage! {
"--warp",
"Does nothing; warp sync is enabled by default. Use --no-warp to disable.",

FLAG flag_fast_and_loose: (bool) = false, or |_| None,
"--fast-and-loose",
"Does nothing. DB WAL is always activated.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention here that the option is deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a placeholder really since --help does not print these options anymore.

@5chdn 5chdn changed the title parity: remove fast-and-loose option. db: remove fast-and-loose option. Jul 4, 2018
@5chdn 5chdn changed the title db: remove fast-and-loose option. db: remove wal disabling / fast-and-loose option. Jul 4, 2018
@5chdn
Copy link
Contributor Author

5chdn commented Jul 4, 2018

@andresilva done 😊

@5chdn 5chdn mentioned this pull request Jul 4, 2018
Merged
@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 4, 2018
@5chdn
Copy link
Contributor Author

5chdn commented Jul 4, 2018 •
edited
Loading

andresilva
andresilva approved these changes Jul 4, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍 Will merge after we deal with deprecation handling.

@5chdn 5chdn removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 9, 2018
@5chdn
Copy link
Contributor Author

5chdn commented Jul 10, 2018

ok, added to deprecated::does_nothing() 👍

dvdplm
dvdplm approved these changes Jul 10, 2018
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm

(When this is merged I need to update parity-common with the changes to kvdb-rocksdb)

andresilva
andresilva reviewed Jul 10, 2018
View reviewed changes
parity/deprecated.rs Outdated
// Removed in 2.0.

if args.flag_fast_and_loose {
result.push(Deprecated::DoesNothing("--fast-and-loose"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, but why DoesNothing instead of Removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess in theory it could "do nothing" but not be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

They're both correct I guess, but DoesNothing seems to be used when there's another opposite option e.g. --warp and --no-warp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. It should be removed.

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 10, 2018
@5chdn
Copy link
Contributor Author

5chdn commented Jul 10, 2018

FFFFFFFFFFFUUU... 🤣

@dvdplm dvdplm mentioned this pull request Jul 10, 2018
Merged
@dvdplm
Merge branch 'master' into a5-cli-fastloose
7b765b8 
* master:
  Delete crates from parity-ethereum and fetch them from parity-common instead (#9083)
  Updater verification (#8787)
  Phrasing, precisions and typos in CLI help (#9060)
@dvdplm
Copy link
Collaborator

dvdplm commented Jul 10, 2018

NOTE We should merge paritytech/parity-common#7 before merging this. If you came here to review, please consider reviewing that one instead.

@dvdplm dvdplm merged commit da5de4a into master Jul 10, 2018
@5chdn 5chdn deleted the a5-cli-fastloose branch July 10, 2018 15:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@andresilva andresilva andresilva approved these changes

@niklasad1 niklasad1 Awaiting requested review from niklasad1

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M2-config 📂 Chain specifications and node configurations.
Projects
None yet
Milestone
2.0 Ethereum
Development

Successfully merging this pull request may close these issues.

Change name/description of fast_and_loose.
4 participants
@5chdn @andresilva @dvdplm @niklasad1