Skip to content

Conversation

@cigaly
Copy link
Contributor

@cigaly cigaly commented Sep 26, 2024

Jira issue HHH-18377

This pull requests is update to #8719


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


private record State(long timestamp, int sequence) {
public State getNextState() {
final long now = instantToTimestamp();
if ( this.timestamp < now ) {
Copy link
Member

Choose a reason for hiding this comment

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

Most of the time, now will be greater than this.timestamp i.e. the previous timestamp, so this code would always create a new random sequence.
This is what I was trying to say with my comment about naming. If I understand the recommendations correctly, we have essentially two options:

  • Always use a random clock sequence
  • Use a random clock sequence only if this.timestamp > now

I don't know what this code is trying to attempt or what is a good strategy, but it makes sense to me to increase the clock sequence when two threads end up with the same timestamp to avoid collisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.timestamp is timestamp of the last state. In any "normal" situation current timestamp (now) should be either equal or larger than timestamp. Possible reasons for current timestamp being less that last one are mentioned in RFC - clock rollbacks or leap second handling. In that case generator will act as if they are equal and increment counter. Only in case if incrementing counter will make it to rollover, timestamp will be incremented and random counter generated.

About naming, AtomicReference holding last state is called lastState, and timestamp, and counter are just fields of State record. I am not sure that there is need to rename them, but it is not a problem if you think that this will make some big difference.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that a random clock sequence should only be generated if the previous timestamp is greater than the current timestamp. In all other cases, the clock sequence should just be incremented by 1.
Or if we decide to always use a random clock sequence, do so unconditionally.
The current code generates a random clock sequence if the current timestamp is greater than the previous timestamp, and increments the clock sequence if the current timestamp is smaller or equal to the previous timestamp. This is exactly opposite of what I understood should happen, which indicates to me that the naming you chose confused you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think that naming is problem, now I've renamed State object properties. The point is that timestamp at the moment of next state generation should normally be either equals to last one (then increment counter) or larger (generate random counter). In some exceptional situation when it is smaller, we simply assume that they are equal and incrementing counter. Only when counter is at its maximum value, timestamp is incremented and random counter is generated. Otherwise it will rollover to zero.

@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Oct 7, 2024

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

| ( getSequence( currentTimestamp ) & 0x3FFFL ) << 48
// LSB bits 16-63 - pseudorandom data
| Holder.numberGenerator.nextLong() & 0xFFFF_FFFF_FFFFL
| ( state.lastSequence & 0x3FFFL ) << 48
Copy link
Member

Choose a reason for hiding this comment

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

Current implementation already ensures that the value is in range, so no need for masking.

Suggested change
| ( state.lastSequence & 0x3FFFL ) << 48
| state.lastSequence << 48

@beikov
Copy link
Member

beikov commented Oct 7, 2024

The implementation looks ok to me now, but I'm having a really hard time figuring out what a good implementation for this is in the first place.
So far, I was trying to follow https://www.ietf.org/archive/id/draft-peabody-dispatch-new-uuid-format-01.html#section-4.3.4 which uses a slightly different algorithm for the clock sequence part i.e. only generate a random clock sequence if the clock runs backwards, whereas your implementation does it the other way around.
I can't judge what a good algorithm is for all this, but I'm kind of disappointed that there is no official guidance or reference algorithm. It seems like implementors have to figure this out for themselves.

@cigaly
Copy link
Contributor Author

cigaly commented Oct 7, 2024

Question is what can guarantee that randomly generated sequence will be larger than previous one? If not, there is chance, small one, but still not impossible, that next UUID will be smaller than last one.
I was looking at RFC section "Monotonic Error Checking". It was published in May 2024, while draft was published in October 2021, and both are mentioning old RFC 4122

@beikov beikov merged commit 8ca2481 into hibernate:main Oct 16, 2024
19 of 20 checks passed
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.

2 participants