-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update virtual_person_id type to uint64. #56
Conversation
Currently the population_offset and total_population in VirtualPersonPool are uint64, while virtual_person_id in VirtualPersonActivity is int64. This could cause an issue when the range covered by virtual person pools is out of range of int64. This is a safe change per https://developers.google.com/protocol-buffers/docs/proto#updating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this requires that you change all the upstream builds as well? I'm assuming this will cause problems in an EDP uses an old version of virtual-people-common?
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tcsnfkx and @wangyaopw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tcsnfkx)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per https://developers.google.com/protocol-buffers/docs/proto#updating:
int32, uint32, int64, uint64, and bool are all compatible – this means you can change a field from one of these types to another without breaking forwards- or backwards-compatibility. If a number is parsed from the wire which doesn't fit in the corresponding type, you will get the same effect as if you had cast the number to that type in C++ (for example, if a 64-bit number is read as an int32, it will be truncated to 32 bits).
So I think it wouldn't cause issue unless a model use population pool with id range > int64.max_value
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tcsnfkx)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wliue)
Currently the population_offset and total_population in VirtualPersonPool are uint64, while virtual_person_id in VirtualPersonActivity is int64. This could cause an issue when the range covered by virtual person pools is out of range of int64.
Update virtual_person_id type to uint64.
This is a safe change per https://developers.google.com/protocol-buffers/docs/proto#updating
This change is