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)
Thu, Dec 19, 6:48 AM
Unknown Object (File)
Thu, Dec 5, 12:38 PM
Unknown Object (File)
Mon, Nov 25, 7:59 AM
Unknown Object (File)
Mon, Nov 25, 7:59 AM
Unknown Object (File)
Nov 23 2024, 8:33 AM
Unknown Object (File)
Oct 30 2024, 10:55 PM
Unknown Object (File)
Oct 24 2024, 5:51 PM
Unknown Object (File)
Oct 21 2024, 7:58 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.