Add UUIDv7 node partitioning#78
Conversation
|
@copilot review this pr |
|
@codex[agent] review this pr |
|
To use Codex here, create a Codex account and connect to github. |
Code Review — Add UUIDv7 Node PartitioningOverall: Solid implementation. The bit-layout math is correct across all three width ranges (≤6, 7–14, 15–16 bits), RFC variant bits are preserved correctly, and the lock-free path is unchanged. All 211 tests pass. Issues1. // Current — throws for bad width
public ushort? GetNodePartitionId(byte nodeIdBitWidth)
{
UuidV7NodePartition.ValidateBitWidth(nodeIdBitWidth); // throws
...
}Callers expecting the same null-return contract will be surprised. Consider returning 2. DI statistics inconsistency in the full // New overload — closure capture (different from existing pattern)
services.AddSingleton<IUuidV7Factory>(sp => new UuidV7Factory(
sp.GetRequiredService<TimeProvider>(),
nodePartition, rng, overflowBehavior,
statistics)); // ← closure, not sp.GetRequiredServiceFunctionally identical since the same instance is registered, but it diverges from the established pattern and can confuse future maintainers. 3. Missing test for the simple 4. What's correct
|
Co-authored-by: dexcompiler <115876036+dexcompiler@users.noreply.github.com>
Pushed follow-ups in |
|
To use Codex here, create a Codex account and connect to github. |
|
Verification follow-up after the Copilot/Codex review pass:
Validation after the follow-up:
Benchmark sample after precomputing masks:
Refreshed GitHub checks are green. |
|
One benchmark nuance worth calling out explicitly: the slightly worse node-partitioned batch result is expected. Batch generation amortizes the timestamp/counter CAS reservation, but node partitioning still has to overlay the configured partition bits into each UUID's rand_b bytes. After a batch reserves counters, the default path mostly benefits from amortized allocation; the partitioned path still pays a tiny fixed per-UUID byte mask/or cost. After a7c2465, the width-dependent mask/shift math is precomputed once in UuidV7NodePartition, so the remaining batch delta is the unavoidable per-UUID byte overlay plus normal benchmark noise. A further specialized partitioned batch writer might shave a little more, but I would only add that complexity with BenchmarkDotNet evidence showing a meaningful and stable win. |
Summary
Validation
Benchmark sample from this machine:
Fixes #68