APP-7232 Fix fragment usage protos #607
Conversation
| int32 fragment_id = 1; | ||
| int64 organizations = 2; | ||
| int64 machines = 3; | ||
| int64 machines_in_current_org = 4; |
There was a problem hiding this comment.
@michaellee1019 do we have a standard on when we're using int32 versus int64? seems pretty inconsistent right now. i realized that int64 casts to a string in the frontend protos, which forces us to do a cast to use the values. i'm leaning towards using int32 unless we're convinced we really need int64
There was a problem hiding this comment.
Huh I did not realize that. I've used int64 for other things and this never came up. I believe you and fine to move to int32. We have quite a while before 2.1 Billion machines use a fragment. I'm optimistic though. Max length is really only the reason.
There was a problem hiding this comment.
FWIW long_type_string a buf configuration option that we could choose to disable. IIRC int64 it will be a BigInt if we turn this off. BigInt has it's own issues, I agree that int32 is probably fine and not going to be the thing that breaks first as we scale
version: v1 plugins: - name: ts out: . opt: generate_dependencies,force_optimize_code_size,long_type_string,ts_nocheck strategy: all
There was a problem hiding this comment.
i think for this use case we should stick with int32, but good to know for the future
There was a problem hiding this comment.
Also @mcous do you think at some point we could use the TS SDK directly? The SDK is already correctly using bigint for example: https://ts.viam.dev/classes/appApi.RegistryItem.html#totalExternalRobotUsage
ehhong
added
the
ready-for-protos
ehhong
removed
the
ready-for-protos
Overview
Bozo move here. Fixing some mistakes that were not caught during review. These values are not being used anywhere yet so breaking change should be okay.