nats-io / jsm.go

JetStream Management Library for Golang
Apache License 2.0
142 stars 23 forks source link

jsm.MsgInfo sequence methods return int #70

Closed bruth closed 4 years ago

bruth commented 4 years ago

This is an observation, but partly a question. I noticed the StreamSequence() and ConsumerSequence() returns an int which implies to me the max size of the stream that can be represented is the max int32 value. I presume a stream is not bound by this upper limit? I would assume max int64 would be the upper bound at least?

ripienaar commented 4 years ago

int max is 64 bits wide though isn't it? But you're right if this ran on a 32bit machine it would be problems

Regardless we get these numbers via JSON and JSON integers are limited to 32bit in practise so it's a bit of a pain :) Other cases where people have this problem - like the google APIs - they send 64bit numbers are strings I don't really know whats the best here.

@derekcollison wdyt?

ripienaar commented 4 years ago

ping @aricart

ripienaar commented 4 years ago

To be clear we can obviously make this work in go<->go even with uint64s but I think I recall @aricart saying this will break Javascript so not sure what we can do about it?

bruth commented 4 years ago

@ripienaar Yes you are right int is as wide as the OS supports. That was actually an oversight on my part.. for whatever reason I thought int defaulted to 32 bits. So actually this may be the desired behavior for cross-arch support since it will be the right width relative to device it is running on.

Regardless we get these numbers via JSON and JSON integers are limited to 32bit

Yes, interestingly the JSON spec doesn't specify this constraint. It simply provides a representation of numbers. The issue comes with interop between languages and how JSON parsers deal with the numbers. JavaScript is particularly bad since it only supports up to 53 bits. It will happily truncate 64 numbers down..

ripienaar commented 4 years ago

Yeah, we definitely need something, for go I think you're right I need to do better, but this leaves other languages up the creek so to speak :(

bruth commented 4 years ago

I should have said this up front, but thanks for creating this library. It has been pleasant to work.

ripienaar commented 4 years ago

Thanks for the feedback :) And we'd welcome any other suggestions, we're looking to design some helpers to make subscribing/creating etc easier - bits of syntax sugar, so any ideas you have would be welcome

bruth commented 4 years ago

Great, I forward ideas if/when they come to me.

Coming back to the above issue, I just saw one bit of inconsistency with the StartAtSequence(s uint64) consumer option which takes an uint64 (and consumer.StartSequence() returns a uint64), so aligning the types with MsgInfo would be good.

More generally, if there was a section on the JS README calling out the "limits" like max messages in a stream, max number of streams, max number of consumers, etc. Obviously any justifications related to those would be useful to understand (whether practical or technical).

ripienaar commented 4 years ago

Yeah I'll go through all these things and make them uint64 where they should be after some chats here about JS

derekcollison commented 4 years ago

JSON output is not limited to 32bits on numbers, depends on OS parsing the JSON.

We should be consistent on APIs for sure and specify size hints to languages that will parse this JSON.

ripienaar commented 4 years ago

JSON output is not limited to 32bits on numbers, depends on OS parsing the JSON.

Yeah so making it work on go is easy, but then what happens with javascript? Anything we can do to make it better for JS, have to ask @aricart

derekcollison commented 4 years ago

I believe this for modern javascript engines still tied to dp. Some engines do real integers IIRC.

Number.MAX_SAFE_INTEGER = 9007199254740991; // Math.pow(2, 53) - 1;

aricart commented 4 years ago

Yes 9007199254740991 is the max safe integer value. I wonder if then we need a generation. That could be another number. I think 9 quadrillion is a really big number. While this will be quoted soon enough I think it will be a long time before we capture that many messages. By my calculation if we are doing 10M messages per second, we have about 1713.69848834 years.

bruth commented 4 years ago

@aricart 😄 Indeed. The practical side to this is equally valid.

aricart commented 4 years ago

Another interesting take would be - if we had that many messages and they were only 8 bytes, we would require 72 petabytes - oh my that is a big disk.

ripienaar commented 4 years ago

Lol. Mainly just want to be sure we will t ha e any truncation or rollovers but I think we are good. I will fix the go side soon

ripienaar commented 4 years ago

fixed the message metadata stuff in https://github.com/nats-io/jsm.go/pull/92 the rest seems in good shape

bruth commented 4 years ago

👍 Thanks