search

cloudevents / spec

CloudEvents Specification
https://cloudevents.io
Apache License 2.0
5.09k stars 584 forks source link

Review Integer mapping: 32 vs. 64 bit #514

Closed alanconway closed 5 years ago

alanconway commented 5 years ago

The CE spec uses 32 bits for Integer - .this is likely the last chance to review that decision.

I raise it because the spec now refers explicitly to the JSON string format for numbers in the definitoin of Integer. It doesn't say anything about JSON integer range (and neither does JSON!), but the practical reality is 52 bits (IEEE double precision mantissa).

Perhaps 64 bits is a safer bet for CloudEvents than 32, so we can handle the de-facto range of JSON-encoded integers into the future. Native 64 bit support is common these days, and where native support is lacking there's always a hack.

I'm not 100% convinced either way but would like to air it one last time before its too late.

(Related to in #513)

clemensv commented 5 years ago

I am not 100% convinced either way, either.

duglin commented 5 years ago

Would it be possible to get a proposal for this before EOD tomorrow? Either a PR or a proposal to close the issue.

cneijenhuis commented 5 years ago

Proposal to close this issue: At this very late stage, IMO, we shouldn't re-evaluate every decision unless we know it is a major concern. The current decision was part of the spec for a long time, and has not caused an issue so far.

duglin commented 5 years ago

Thanks @cneijenhuis , I tend to agree that unless we have new information we shouldn't reopen old/resolved issues. Anyone have an concerns with closing this?

alanconway commented 5 years ago

No objection from me.

duglin commented 5 years ago

LOL I didn't mean for you to close it immediately, but obviously since you're the owner of the issue you can do so :-) - and the issue-tracker in me is glad, but I was going to wait for the call to give people time to think about it. So, for others... if you believe there is a reason to keep it open just speak up and we can reopen if needed.

alanconway commented 5 years ago

Oops. Itchy trigger finger.

On Tue, Oct 8, 2019 at 9:01 AM Doug Davis notifications@github.com wrote:

LOL I didn't mean for you to close it immediately, but obviously since you're the owner of the issue you can do so :-) - and the issue-tracker in me is glad, but I was going to wait for the call to give people time to think about it. So, for others... if you believe there is a reason to keep it open just speak up and we can reopen if needed.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/cloudevents/spec/issues/514?email_source=notifications&email_token=AB3LUXV3LPYNKHLFEQD4XTTQNSACNA5CNFSM4I2C3OLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAUCJ2Q#issuecomment-539501802, or mute the thread https://github.com/notifications/unsubscribe-auth/AB3LUXS3ITMPAZDD2AXFEMLQNSACNANCNFSM4I2C3OLA .

black-snow commented 1 year ago

I'd like to add to this:

With 32 bit signed integers we can't really use Integer for UNIX timestamps. We'd have to drop back to String but then we end up with comparison issues. I wanted to add a custom extension for a revision number (for sequence has its issues in my case) but the 32 bit limit makes it hard. So I can either choose to ignore it (meh) or use String and say "whoever reads this, it's always numbers in there". @alanconway @duglin

duglin commented 1 year ago

Given the current state of the spec, I think String may be your only option. Maybe by 2038 we'll have a new rev of the spec and make INTs 64 bit :-)