Updated V7 generator to Draft04.

[bgadrian]

Updated V7 generator to enforce the monotonic property for ids generated in the same timestamp. Updated tests and go docs.

[codecov-commenter]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (6088057) compared to base (7b40032). Patch coverage: 100.00% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #112 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 4 4 Lines 473 498 +25 ========================================= + Hits 473 498 +25 ``` | [Impacted Files](https://codecov.io/gh/gofrs/uuid/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gofrs) | Coverage Δ | | |---|---|---| | [generator.go](https://codecov.io/gh/gofrs/uuid/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gofrs#diff-Z2VuZXJhdG9yLmdv) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gofrs). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gofrs)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[cameracker]

Thanks for the submission @bgadrian! Would you mind rebasing this branch with master?

Also, thoughts on the code coverage loss?

[bgadrian]

@convto If the user does not generate batches then it means it calls the method once each ms, so contention is not a problem. And without the monotonic seq the UUID is not v7 according to the specification.

As alternative we could refactor or add a new method that uses atomic package instead of a mutex to improve the concurrency If needed.

On Tue, Jan 10, 2023, at 07:10, YuyaOkumura wrote:

@.**** commented on this pull request.

In generator.go https://github.com/gofrs/uuid/pull/112#discussion_r1065338900:

u[1] = byte(ms >> 32)

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

• //The 6th byte contains the version and partially rand_a data.
• //We will lose the most significant bites from the clockSeq (with SetVersion), but it is ok, we need the least significant that contains the counter to ensure the monotonic property
• binary.BigEndian.PutUint16(u[6:8], clockSeq) // set rand_a with clock seq which is random and monotonic

It may be better to make the API user-selectable whether to consider batch generation or not. This is because getClockSequence performs a mutex lock, and using it will result in worse performance and reduced generation capability. For non-batch generation use cases, it is probably undesirable to have getClockSequence run, so a user-selectable API might be better.

(For example, if breaking changes are allowed, adding isBatch to the NewV7 argument.)

— Reply to this email directly, view it on GitHub https://github.com/gofrs/uuid/pull/112#pullrequestreview-1241529701, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGKUMMCHZL5SX2JPLMRRSTWRTVLTANCNFSM6AAAAAATPXQRRM. You are receiving this because you were mentioned.Message ID: @.***>

/***/

⚛ Bledea-Georgescu Adrian 🖧 Sofware Engineer ✍ https://coder.today 📧 @.*** 🖥 github.com/bgadrian 💁 linkedin.com/in/bgadrian https://www.linkedin.com/in/bgadrian/

[convto]

@bgadrian The Rev04 draft monotonic counter specification was defined with SHOULD and MAY requirement levels, so I wanted to give the user a flexible option. But as you commented, it doesn't seem to be much of a problem.

Thanks for your reply!

[bgadrian]

Thanks for the submission @bgadrian! Would you mind rebasing this branch with master?

Also, thoughts on the code coverage loss?

Hello, I have addressed the comments, restored the code coverage and rebased with the master.

[cameracker]

Hi @bgadrian ! I'm still planning on reviewing and accepting this contribution but haven't had the time to study the new updates to the draft to check for correct implementation. I'll do my best to get to it this week, I appreciate your patience.

[cameracker]

Also, I am tentatively planning on putting out a release as soon as this is merged.

Another thing @bgadrian , a couple of PRs have been merged to Master. One meaningful PR is the change to how coverage is collected. The addition of Generator options may have a small impact on this PR but unclear. Would you rather me update this PR for you or would you like to process these updates yourself?

[cameracker]

Ok, I completed a review. Sorry it took me so long. And thank you so much for the contribution.

Last request: Could we update the README.md to reflect which version of the Draft we're implementing for v6 and v7?

As an overall comment, I believe this PR correctly implements the v6 and v7 UUIDs to specification, but I'm getting the sense that we're not being as clear as we could be on which "MAY" "SHOULD" behaviors we chose to address in this implementation and worry that some user is going to pick up those UUIDs and run into "undefined behavior" sort of problems. What do you think? Should we be more explicit on our approach anywhere? @convto @bgadrian @dylan-bourque

[bgadrian]

I have merged with the latest master, updated the Readme and addressed some comments.

As for being explicit or not, I think the v4 specifications is not a MAY or SHOULD, it is mandatory (SHOULD) to ensure the monotonic property

Additionally, care SHOULD be taken to ensure UUIDs generated in batches are also monotonic. That is, if one-thousand UUIDs are generated for the same timestamp; there is sufficient logic for organizing the creation order of those one-thousand UUIDs.

But the specs does not enforce which algorithm to use

MAY utilize a monotonic counter

The draft states that

For single-node UUID implementations that do not need to create batches of UUIDs,

This indeed makes the Batching optional, which is confusing, but the problem is that, the users will not know if they need or not batching most likely, I presume most real world scenarios of generating UUIDs are based on events that cannot be controlled (new users, new resources), so the "need" or "not need" of batching cannot be guaranteed, only presumed that is ok 99.99% of the time.