search

eclipse-uprotocol / up-cpp

uProtocol Language Specific Library for C++
Apache License 2.0
15 stars 24 forks source link

This PR adds UUID builder for uprotocol uuidv8 #151

Closed BishrutSubedi closed 2 months ago

BishrutSubedi commented 2 months ago

Add uuid builder and it's unit test

TODO:

Convert from uuidv8 to uuidv7 Follow up on thread safety

stevenhartley commented 2 months ago

that could clean this up, and I think that the current

@gregmedd given the change now to move to UUIDv7 for the next release, we don't need to share state anymore as rand_a and rand_b is re-generated at each call to create()

billpittman commented 2 months ago

Looking at the code and RFC 9562 (referenced in the spec), this looks more like v7 instead of v8. Is the title a typo ?

If this is supposed to be v8, it seems like all the variable names are aligning with the v7 field names instead of the v8 field names (ie unix_ts_ms vs custom_a, etc.).

BishrutSubedi commented 2 months ago

Looking at the code and RFC 9562 (referenced in the spec), this looks more like v7 instead of v8. Is the title a typo ?

If this is supposed to be v8, it seems like all the variable names are aligning with the v7 field names instead of the v8 field names (ie unix_ts_ms vs custom_a, etc.).

The spec were recently updated to use uuidv7. The previous spec of uuidv8 closely aligned with uuidv7. The implementation should be similar. I'll go over spec to see if there are any specific changes

gregmedd commented 2 months ago

On the change from v8 to v7, I lean towards proceeding with this PR as-is. I understand it is the wrong UUID version. We can take a follow-up task to correct the implementation back to uuid v7 and evaluate if any of the other modules need to change at the same time (e.g. the validator).

BishrutSubedi commented 2 months ago

It looks like the counter field is replaced by rand_a. The remaining bits allocation are similar.

gregmedd commented 2 months ago

Tracking change to UUIDv7 as #156

BishrutSubedi commented 2 months ago

In addition to what @gregmedd said above, it seems to me if the counter freezees, shouldn't that be some sort of error? If someone generates 5000 UUID's within one ms (possible I think), some of them would be identical ... right?

@billpittman I agree with this, but strictly going by the previous spec, counter is expected to freeze. This is a valid point though. I think throwing exception here can cause software calling build() function to crash, unless they check the counter value and handle it appropriately. May be returning std:nullopt would be better than raising exception. We're looking to change this to v7, but this is good discussion to have soon