Page MenuHomePhabricator

Use "null", not "-1", as a local "no version" marker when performing intracluster repository sync
ClosedPublic

Authored by epriestley on Jan 28 2019, 7:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 5:20 PM
Unknown Object (File)
Wed, Mar 27, 5:20 PM
Unknown Object (File)
Sat, Mar 23, 12:33 PM
Unknown Object (File)
Sat, Mar 23, 12:33 PM
Unknown Object (File)
Jan 22 2024, 4:36 PM
Unknown Object (File)
Jan 6 2024, 5:34 PM
Unknown Object (File)
Dec 21 2023, 4:35 PM
Unknown Object (File)
Dec 7 2023, 1:44 AM
Subscribers
None

Details

Summary

Ref T13242. See https://discourse.phabricator-community.org/t/out-of-range-value-for-column-deviceversion/2218.

The synchronization log column is uint32? and -1 doesn't go into that column.

Since we're only using -1 for convenience to cheat through $max_version > $this_version checks, use null instead and make the checks more explicit.

Test Plan

Reproducing this is a bit tricky and I cheated fairly heavily to force the code down this pathway without actually building a multi-device cluster, but I did reproduce the original exception, apply the patch, and observe that it fixed things.

Diff Detail

Repository
rP Phabricator
Branch
synclog1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21726
Build 29643: Run Core Tests
Build 29642: arc lint + arc unit

Event Timeline

src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
212

I also traced this variable down the stack, nothing else uses it except the logging which is running into an issue.

Thanks for fixing this. When I get back let's make sure I can repro these cluster issues in my dev environment and update the docs (and/or finish my workflow script for automatically setting up all this state for dev environments, which is arguably better than documentation).

This revision is now accepted and ready to land.Jan 29 2019, 2:44 AM

I think reproducing this "legitimately" ultimately requires genuinely setting up two actual repository hosts so it's probably not reproducible in a local Phacility development configuration. I'm not sure what we want to do about that long-term, maybe spinning up a private cluster as a testbed is the best approach to look towards.

(We should definitely get your dev environment to a state where repository cluster stuff works, though, since 95% of stuff doesn't need 2+ hosts to repro/test/develop.)

This revision was automatically updated to reflect the committed changes.