This is an archive of the discontinued Mercurial Phabricator instance.

noverify: add extension to skip repo verification
AbandonedPublic

Authored by phillco on Dec 7 2017, 6:49 PM.

Details

Reviewers
durham
quark
Group Reviewers
Restricted Project
Commits
rFBHGX1b85ea1826cc: noverify: add extension to skip repo verification
Summary

Verify can be extremely slow on large repos, as it tries to inspect every
commit, possibly downloading trees for each one, so this extension disables it.

This also disables the verification step of rollback.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Dec 7 2017, 6:49 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 7 2017, 6:49 PM
phillco updated this revision to Diff 4227.Dec 7 2017, 6:50 PM
durham requested changes to this revision.Dec 7 2017, 7:42 PM
durham added a subscriber: durham.

Back in your queue for tests and a commit message

This revision now requires changes to proceed.Dec 7 2017, 7:42 PM
singhsrb added inline comments.
hgext3rd/noverify.py
30 ↗(On Diff #4227)

Assuming 0 is to notify success, should this actually be 0?

quark added a subscriber: quark.EditedDec 7 2017, 8:57 PM

I guess the main issue here is the "verify after rollback". Verification might still be useful especially for the last few revisions.

IDI is working on adding --rev to verify. It's probably better to still verify the tip revision instead of skipping everything.

hgext3rd/noverify.py
29 ↗(On Diff #4227)

Missing _

singhsrb added inline comments.Dec 7 2017, 9:51 PM
hgext3rd/noverify.py
29 ↗(On Diff #4227)

That's a good spot!

phillco added inline comments.Dec 13 2017, 1:06 PM
hgext3rd/noverify.py
30 ↗(On Diff #4227)

Ah, sometimes we use () (like in debugpackstatus), and sometimes _(). When should we use which?

phillco edited the summary of this revision. (Show Details)Dec 13 2017, 1:19 PM
phillco updated this revision to Diff 4425.

Added a test, description and moved to config.

I like Jun's idea of looking at the last few commits as a hueristic, but here's a version we can get out quickly in the interim.

Why not disable only manifest verification, which is the slow thing here? changelog verification since that can't be re-downloaded on-demand seems reasonable to me. This, of course, is probably effective enough but just kicsk the can down the road if there's an actual corruption issue in a revlog.

I can do that, we just wanted to get a MVP out ASAP since people are hurtin'.

OK, extracting just the manifest verification will take a bit of upstream refactoring. I'll send an upstream change to do that + a followup to scope this to just changesets. But people are still running verifies that take 4 days and make tons of packfiles, so I'm still +1 on landing this while that's happening.

(Need to look at https://phabricator.intern.facebook.com/D6558110 too which is touching verify)

durham accepted this revision.Dec 14 2017, 5:24 PM
This revision is now accepted and ready to land.Dec 14 2017, 5:24 PM
quark requested changes to this revision.Dec 14 2017, 5:24 PM

Wrapping hg recover to skip doing verify is probably a better approach.

This revision now requires changes to proceed.Dec 14 2017, 5:24 PM
phillco updated this revision to Diff 4463.Dec 15 2017, 1:51 AM
phillco updated this revision to Diff 4464.

Based on Messenger discussion, this version just skips the manifest verification and future steps which require it, but keeps changeset verification. Can probably get by without upstream refactor.

durham accepted this revision.Dec 15 2017, 1:57 AM

Let's default the behavior to on. Since the only reason the extension would be enabled is to disable the behavior. Then we can just use the config knob later if we need more flexibility.

phillco updated this revision to Diff 4466.Dec 15 2017, 1:59 AM