-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib/db, lib/syncthing: Repair db once on upgrade (ref #6425, #6427) #6429
Conversation
lib/syncthing/syncthing.go
Outdated
if prevVersion != "" { | ||
l.Infoln("Detected upgrade from", prevVersion, "to", build.Version) | ||
// Check and repair metadata and sequences once immediately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just do checkrepair on every upgrade for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing it on all upgrades including RCs feels like a lot of churn (e.g. considering the 11 RCs last month). And we need it to happen now on v1.4.1-rc.2. So maybe do it on stable updates and v1.4.1-rc.2 explicitly could be a middle way. Or am overthinking and for RC users it's fine to do it on every upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I didn't consider that we want it to happen between-rc:s this time. However, I wonder if the correct thing here isn't to actually run this on every upgrade, even rc-to-rc. The criteria then would be to split on +
and just string-equals compare the thing before the plus (if any).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the full index transfer is a lot heavier than the checks we do, so if anything that would be the part I think we can remove nowadays...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure using lib/upgrade to compare adds much, the version compare should be equivalent to the string compare as we just do an equals check anyway? But it also doesn't hurt 🤷♂
* release: lib/db, lib/syncthing: Repair db once on upgrade (ref syncthing#6425, syncthing#6427) (syncthing#6429) lib/db: Fix removeFromGlobal and no filenames in error (fixes syncthing#6427) (syncthing#6428) lib/db: Remove emptied global list in checkGlobals (fixes syncthing#6425) (syncthing#6426)
Both #6425 and #6427 require a metadata (respectively sequence) recalc. This adds an exported method to
db.Lowlevel
to trigger that, which is then done on startup if an upgrade with a previous version <1.4.2 is detected.I moved the code for the recalculation from set.go to lowlevel.go and made them methods of
*Lowlevel
(which before got passed as arg). This is entirely copy&paste, except that the previousrecalc
fixture now is now a method calledgetMetaAndCheck
.I manually tested running a binary from this PR on a db from 1.4.0.