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

Client: Java ID generator helper #1347

Merged
merged 10 commits into from
Jan 22, 2024
Merged

Client: Java ID generator helper #1347

merged 10 commits into from
Jan 22, 2024

Conversation

kprotty
Copy link
Contributor

@kprotty kprotty commented Dec 11, 2023

Creates a static helper function in the java client for generating thread-safe, unique, and monotonic u128 IDs to use for Accounts and Transfers.

The ULID spec notes that IDs within the same millisecond dont need to be
ordered: https://github.com/ulid/spec?tab=readme-ov-file#sorting
Overflow should happen relative to the 80-bit random generated at the
timestamp rather than relative to a separate counter at the timestamp
which is applied to the random.

This provides sort order guarantee using the recommended approach from
the spec: https://github.com/ulid/spec?tab=readme-ov-file#monotonicity
@kprotty kprotty marked this pull request as ready for review December 13, 2023 03:11
@kprotty kprotty requested a review from batiati December 13, 2023 03:12
Copy link
Contributor

@batiati batiati left a comment

Choose a reason for hiding this comment

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

Nice and elegant implementation!!!

Can we push the scope of this PR a little further and review the samples and docs to mention/use ULID where it's applicable?

}

var ulidB = new DecodedULID(UInt128.ULID());
ulidB.assertMonotonicSince(ulidA);
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 also use UInt128.asBigInteger to compare both ULIDs?
This will assert that the binary representation is really what we expect at Zig's side (a monotonically increasing u128 little-endian).

var bigIntA = UInt128.asBigInteger(...);
var bigIntB = UInt128.asBigInteger(...);
assertTrue(bigIntB.compareTo(bigIntA) > 0); 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of: asBigInteger interprets the bytes as 2 little-endian u64 values of a u128. ULID however stores timestamp as {u32, u16} and random as u80, both big-endian. We can instead interpret the bytes directly via new BigInteger(ULID()) since it assumes big-endian. This also means Zig side might not see it as monotonically increasing little-endian.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ULID has been renamed to ID and it stores the individual fields in little-endian which allows use of asBigInteger.

For setups which download both `zig/zig` and `zig/zig.exe` for convenience
(i.e. windows WSL), this ensures the Java CI uses the correct one instead of
relying on shell to append `.exe` (which doesn't happen since non `.exe`
version exists).
@kprotty kprotty changed the title Client: ULID helper (wip) Client: Java ULID generator helper Dec 14, 2023
ULID spec requires storing the values as big endian which means, when interpreted
by a replica on the Zig side, the `u128` ID is no longer monotonically incrementing.
This resorts in unique but effectively randomly-ordered IDs which doesn't take
advantage of data/lookup optimizations in the storage engine.

Instead, the Java client now exposes a generic `ID()` helper (instead of specifically `ULID`)
which allows us to return u128 bytes in little-endian format while still using the core
ideas around uniqueness & monotonicity from ULID underneath.
@kprotty kprotty changed the title Client: Java ULID generator helper Client: Java ID generator helper Jan 19, 2024
@kprotty kprotty requested a review from batiati January 19, 2024 04:40
Switches to using the 48 LSB of timestamp instead of 16 LSB and 32 MSB. Also updates the data-modeling
documentation to give note about storing ULID fields in little endian.
Copy link
Contributor

@batiati batiati left a comment

Choose a reason for hiding this comment

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

Just two minor comments, but LGTM other than that!

Standardize upper-case hex literals and refer to ULID spec as `ID()`'s inspiration.
batiati
batiati previously approved these changes Jan 20, 2024
Copy link
Contributor

@batiati batiati left a comment

Choose a reason for hiding this comment

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

Thanks King ✌️

@kprotty kprotty added this pull request to the merge queue Jan 22, 2024
Merged via the queue into main with commit cf4f43c Jan 22, 2024
25 checks passed
@kprotty kprotty deleted the king/java-ulid branch January 22, 2024 14:43
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