proxy: Change the imgid to uint64 #1777
Conversation
There was a problem hiding this comment.
Does this break any existing users? And if it does, do we need to worry about version negotiation and/or caller opt-in by calling newer versions of the methods? Ugh.
I think you are maintaining the known user / all the known users, so that’s really up to you.
Apart from the above, implementation LGTM.
|
I don't think so, though I didn't actually yet cross-check with my code. Marking draft until I do so. (and I may look at adding some proper reverse dependency testing) My assumption here is that these uint32/uint64 things are only internal to the proxy - what gets sent on the wire is JSON, and so we're just expanding the space of internal values. Now, the Rust client code here also hardcodes uint32, so it would fail if one of the values actually did get larger, but that can be fixed later. |
|
That’s a fair point, this should be completely invisible on the wire until we actually overflow 32 bits — and once we do, both the old and new version would presumably fail. The only worry would be a consumer that connects to the new version, and casts the float to 32 bits, silently discarding upper bits, and then referring to the wrong handle. |
|
Added test code here #1781 - let's merge that, then use this PR as a test for the tests to validate that this change doesn't break the API. |
|
Just noting that #1781 was closed in favor of cevich@f4b4eb0, which looks to have merged. Can this be rebased and re-tested at this point or is it "hurry up and wait" for something else now? |
mtrmac
added
the
kind/bug
|
A friendly reminder that this PR had no activity for 30 days. |
cgwalters
added
the
experimental-image-proxy
cgwalters
force-pushed
the
proxy-imageid-overflow
branch
2 times, most recently
from
588bd00
to
2538b95
Compare
|
I've rebased 🏄 this now that we have proper revdep testing. But let's merge #2029 first. |
In PR review for a different issue, the question of what happens if we hit overflow for the imageid serial was hit. This feels pretty unlikely; if I did the math right, it'd require opening an average of 136 images per second to overflow it in a year. Nevertheless, in practice what we're sending on the wire is just a JSON number, and if we extend this to the "max safe JSON number" of 2^53, it'd take 285,616,414 images per second to overflow in a year, going from implausible to probably impossible. With a bit more work of course, we could make this a sparse mapping and reuse freed numbers, but eh. Signed-off-by: Colin Walters <walters@verbum.org>
mtrmac
force-pushed
the
proxy-imageid-overflow
branch
from
2538b95
to
f7dc084
Compare
In PR review for a different issue, the question of what happens if we hit overflow for the imageid serial was hit. This feels pretty unlikely; if I did the math right, it'd require opening an average of 136 images per second to overflow it in a year. Nevertheless, in practice what we're sending on the wire is just a JSON number, and if we extend this to the "max safe JSON number" of 2^53, it'd take 285,616,414 images per second to overflow in a year, going from implausible to probably impossible.
With a bit more work of course, we could make this a sparse mapping and reuse freed numbers, but eh.