HHH-19935 UuidVersion7Strategy can generate cosecutive UUIDs that are equal

[cigaly]

Jira issue HHH-19935 UuidVersion7Strategy can generate cosecutive UUIDs that are equal

Increment to current sequence number is properly generated so that it never can be zero.

---

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.

---

[beikov]

I'm not sure what the purpose of this code was exactly, so please be so kind and add some code comments that explain this a bit. Since the maximum value that can be returned by nextLong( 0xFFFF_FFFFL ) is 0xFFFF_FFFEL, I think this should be fine, no?

Suggested change

	final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFEL ) + 1;
	final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFFL ) + 1;

But more importantly, like I asked in the other PR, what aren't we just always using a random sequence? Citing my other comment from before here again:

Yes, please create a Jira issue and a PR. Also, while you're at that, can you please add a javadoc comment explaining the * 0.004096 in the nanos method?

I'm also a bit surprised about the 0xFFFF_FFFFL constant now that I'm looking at it again. The javadoc of the class mentions a 62 bit pseudo-random counter, but here we just add a 16 bit random value to an existing sequence. What is the reason for this again? We could always create the full random 62 bits, but I guess there is an advantage to this process?

[cigaly]

You are right, bound value should not be changed, only 1 should be added to generated random.

If we replace current

				final long nextSequence = lastSequence + Holder.numberGenerator.nextLong( 0xFFFF_FFFFL ) + 1;

with

final long nextSequence = Holder.numberGenerator.nextLong( 0x3FFF_FFFF_FFFF_FFFFL + 1 );

then

				return nextSequence > MAX_RANDOM_SEQUENCE
						? new State( lastTimestamp.plusNanos( 250 ), randomSequence() )
						: new State( lastTimestamp, nextSequence );

should be replaced with

				return lastSequence >= nextSequence
						? new State( lastTimestamp.plusNanos( 250 ), randomSequence() )
						: new State( lastTimestamp, nextSequence );

in order to ensure monotonicity. Condition lastSequence >= nextSequence will be true much oftern than nextSequence > MAX_RANDOM_SEQUENCE. This will cause nanos part of state to grow and not unlikely will have influence on millis part as well if large number of UUIDs is generated in very short period of time. If you think that this is OK, I can make changes above.

[beikov]

I'm having trouble understanding why we are even bothering with all this. PostgreSQL for example always uses random 62 bits. Why can't we do that as well? Chances of a collision should be small to non-existent anyway AFAIU. Am I missing something?

[cigaly]

OK, here is version that will generate completely random 62 bits

[beikov]

Thanks, I would be interested though why you chose this somewhat more complicated path in the beginning though. Maybe I'm missing something?

[beikov]

So the nextSequence must be greater than the lastSequence to guarantee monotonicity? And the addition of 250 is because that is the lowest sensible value to add due to using only the upper 12 bit of the nanos? Since the highest potential nano value 999_999 has 20 bits which, I would assume that the smallest sensible value to add is 2^8 i.e. 256. Can you please explain the reasoning in a code comment?