nats-io / nats-architecture-and-design

Architecture and Design Docs
Apache License 2.0
177 stars 20 forks source link

[modifying] ADR-1: adding unsigned 64 bit values #16

Closed scottf closed 2 years ago

ripienaar commented 2 years ago

Seems to me this information would be better kept in the schemas where its more likely to be maintained and the first place someone looks.

We can do it like this:

diff --git a/schema_source/jetstream/api/v1/definitions.json b/schema_source/jetstream/api/v1/definitions.json
index 96570a7..351267c 100644
--- a/schema_source/jetstream/api/v1/definitions.json
+++ b/schema_source/jetstream/api/v1/definitions.json
@@ -5,6 +5,11 @@
   "description": "Shared definitions for the JetStream API",
   "type": "object",
   "definitions": {
+    "uint64": {
+      "$comment": "unsigned int 64",
+      "type": "integer",
+      "minimum": 0,
+      "maximum": 18446744073709551615
+    },
     "stream_source": {
       "type": "object",
       "description": "Defines a source where streams should be replicated from",
@@ -16,8 +21,7 @@
         },
         "opt_start_seq": {
           "description": "Sequence to start replicating from",
-          "type": "integer",
-          "minimum": 0
+          "$ref": "#/definitions/uint64"
         },
         "opt_start_time": {
           "description": "Time stamp to start replicating from",

Then after building the schemas it looks like this:

            "opt_start_seq": {
              "description": "Sequence to start replicating from",
              "$comment": "unsigned int 64",
              "type": "integer",
              "minimum": 0,
              "maximum": 18446744073709552000
            },

We have many different ints in the api, int, int32, int64, uin64 etc, maintaining a document like this will be too hard - adding a 3rd place to put this information - improving the schemas is best.

ripienaar commented 2 years ago

Things are a bit funky around top of uint64 so probably some languages will behave strangely - even here see I put 18446744073709551615 but JS turns that into 18446744073709552000

scottf commented 2 years ago

Things are a bit funky around top of uint64 so probably some languages will behave strangely - even here see I put 18446744073709551615 but JS turns that into 18446744073709552000

The only option would be to remove uint64 from the server. As you have mentioned JS only handles 53 bits of information, but the server is capable of sending a larger number, even as unlikely as it may be to have 18 quintillion (I looked that up!) messages.

I vote for both the schema and this document to be kept up to date, as this will be a quick reference for client developers. Maybe we can generate this document from the schema?

ripienaar commented 2 years ago

@derekcollison I think the vote here is any time you edit a NATS Server data type you should update this ADR. Please comment.

My proposal is as I maintain the Schemas I maintain a single point of truth after and any interested developer - and the NATS CLI - gains immediate benefit by having more correct information and validation in one place.

Clearly the NATS CLI is not going to be reading this ADR to validate user input - it can read schemas though, hence my vote is to improve the schemas.

ColinSullivan1 commented 2 years ago

I agree, the schema seems like the right place for field definitions like this. Behavior around handling these messages would fit in an ADR though...

scottf commented 2 years ago

@aricart @ripienaar I simplified the ADR based on our discussion. Do we need to do anything else?

ripienaar commented 2 years ago

@aricart @ripienaar I simplified the ADR based on our discussion. Do we need to do anything else?

There is no very little of value here in my oppionion, as in, nothing here really is actionable? I wonder if this could be an update to ADR-1 now instead? (ADR-1 needs a general revisit imo)

Who will update the schemas?

ripienaar commented 2 years ago

As in I imagine as a entry point for new client developers ADR-1 should be the starting point, so makes sense to call this out as a general point there of gotchas to keep in mind?

scottf commented 2 years ago

I wonder if this could be an update to ADR-1

Sounds reasonable to me. I'll drop this branch/PR and combine my comments into that.

ripienaar commented 2 years ago

I think I've basically superceded this PR in https://github.com/nats-io/nats-architecture-and-design/pull/28, sorry I am not trying to dismiss your work - its just ADR-1 was frankly embarrasingly bad and of no use and out of date. So adding to it in its current state would not really move the needle much. So I rewrote it entirely.

scottf commented 2 years ago

incorporated into https://github.com/nats-io/nats-architecture-and-design/pull/28