Page MenuHomePhabricator

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

Authored by epriestley on Mon, Jan 28, 7:31 PM.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mon, Jan 28, 7:31 PM
epriestley requested review of this revision.Mon, Jan 28, 7:33 PM
epriestley added inline comments.Mon, Jan 28, 7:34 PM
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).

amckinley accepted this revision.Tue, Jan 29, 2:44 AM
This revision is now accepted and ready to land.Tue, Jan 29, 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.