Skip to content
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 UUIDv7 implementation with RFC Draft Rev 03 spec #99

Merged
merged 2 commits into from Sep 9, 2022

Conversation

convto
Copy link
Contributor

@convto convto commented Jun 5, 2022

Currently, UUIDv6 and UUIDv7 implementations follow RFC draft rev 02 [1].
However, it has already been updated by RFC draft rev 03 [2] in March 2022.

Rev3 updates v6 and v7 and v8 specifications, but the v6 changes seem to
have no impact on implementation. Therefore, v6 and (unimplemented) v8 are
not affected, so only the UUIDv7 implementation will be updated.

[1] https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-02
[2] https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-03

In rev 03, complex sequences and variable precision timestamps have been removed from the specification.
The implementation is now simple and lock-free. It looks good.

This PR has breaking changes to the signature of the exported function NewV7() and Gen.NewV7() ,
but this does not seem to need consideration. I have referenced the following comment.

uuid/generator.go

Lines 92 to 97 in 028e840

// This is implemented based on revision 02 of the Peabody UUID draft, and may
// be subject to change pending further revisions. Until the final specification
// revision is finished, changes required to implement updates to the spec will
// not be considered a breaking change. They will happen as a minor version
// releases until the spec is final.
func NewV7(p Precision) (UUID, error) {

@convto convto force-pushed the update-v7-with-draft-03-spec branch 2 times, most recently from f4883ec to c5459d3 Compare June 6, 2022 04:48
@convto
Copy link
Contributor Author

convto commented Jun 10, 2022

@theckman
I saw you had recently implemented a Rev02 draft on #93 PR!
I think you seem like a very good reviewer of this PR.

If you're the right member for the review, I'd appreciate your review if you have the time needed. 🙇

@convto
Copy link
Contributor Author

convto commented Jul 5, 2022

I checked with gophers slack, @theckman seems to be in a situation where he can't do reviews...

@convto
Copy link
Contributor Author

convto commented Jul 5, 2022

@cameracker
I see you have contributed a lot to this project!
If you have enough time, I would appreciate it if you could review this PR. 🙇

generator.go Outdated
return Nil, err
}

tn := g.epochFunc()
ms := uint64(tn.Unix())*1e3 + uint64(tn.Nanosecond())/1e6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify this by just doing UnixMilli()?

Copy link
Contributor Author

@convto convto Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it seems to be simpler to use UnixMilli() here!

The original code was receiving and processing sec and nanosec for flexibility, and I was writing the same way without thinking about it in detail.

uuid/generator.go

Lines 376 to 384 in edd511b

sec, nano, seq, err := g.getV7ClockSequence(MillisecondPrecision)
if err != nil {
return Nil, err
}
msec := (nano / 1000000) & 0xfff
d := (sec << 28) // set unixts field
d |= (msec << 16) // set msec field

Since we no longer need to worry about timestamp accuracy flexibility, it looks like we can use UnixMili().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to use UnixMilli() in this commit.
5279295

u[2] = byte(ms >> 24)
u[3] = byte(ms >> 16)
u[4] = byte(ms >> 8)
u[5] = byte(ms)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we benefit any from using the binary.BigEndian.PutUint32 interface that we use for the v6? As written makes sense given the lack of a PutUint48 to cover the size of the time in milliseconds..

Copy link
Contributor Author

@convto convto Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you commented, I wrote this code because PutUint48 is not supported. v6 may have similar conditions as far as the specification is concerned.

However, there is a slight difference in v6.
The v6 specification specifies 32bit time_high , 16bit time_mid, 16bit time_low_and_version separately, which may have the effect of expressing the layout more explicitly.

(v7 is specified in the specification as a single field, unix_ts_ms, so I have taken the approach described in the code.)

Copy link
Contributor

@cameracker cameracker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get the build to run?

I see the passing build in the upstream repo, but would you mind rebasing to get the tests to run on two latest versions of go? The comments I left are questions, willing to merge with no changes but wanted to at least ask.

Currently, UUIDv6 and UUIDv7 implementations follow RFC draft rev 02 [1].
However, it has already been updated by RFC draft rev 03 [2] in March 2022.

Rev3 updates v6 and v7 and v8 specifications, but the v6 changes seem to
have no impact on implementation. Therefore, v6 and (unimplemented) v8 are
not affected, so only the UUIDv7 implementation will be updated.

[1] https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-02
[2] https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-03
@convto convto force-pushed the update-v7-with-draft-03-spec branch from db76638 to 5279295 Compare September 9, 2022 04:14
@convto
Copy link
Contributor Author

convto commented Sep 9, 2022

@cameracker
I don't have write access to the project, Could the maintainer members do the merging and releasing?

@cameracker cameracker merged commit f267b3d into gofrs:master Sep 9, 2022
@cameracker
Copy link
Contributor

Thank you so much for the submission @convto :) It's nice to have the WIP versions of the new uuid features in the library. Thanks for keeping them up to date 👍

@convto convto deleted the update-v7-with-draft-03-spec branch September 11, 2022 06:35
kattrali added a commit to bugsnag/bugsnag-go that referenced this pull request Oct 12, 2022
replace the gofrs uuid lib as it broke support for old versions of go in
a recent commit (gofrs/uuid#99) and has no mechanism for version control
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants