feat: dynamic chunkindex shard prefix#111
Open
woutervanranst wants to merge 27 commits into
Open
Conversation
Azure blob name prefixes for chunk-index shards are not segment-aligned (chunk-index/aa must match aa, aa0, aa3f). The existing ListAsync appends a trailing slash, so add ListAsync(directory, namePrefix) with a default client-side-filter implementation, a native Azure override, a recording override on the in-memory fake, and a ChunkIndexLists counter on the counting decorator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shards start at the 2-char prefix layout and split 16-way by the next hex char when they exceed MaxShardEntryCount (10k) at flush time, recursively and unevenly per subtree. Only non-empty shards are written; the layout is self-describing from blob existence. - Routing: parent-wins walk — the shallowest existing shard on a hash's prefix path is authoritative; a crashed split leaves the parent intact and unpublished children shadowed, so reads stay correct at every crash point and a flush retry re-splits and converges. - Lookup: per-root coverage claims in loaded_prefixes (validated at the latest snapshot, zero remote calls when warm); cold path = one subtree listing per root deciding empty-vs-descend, etag revalidation from the listed etag (no download). - Flush: per-root gates; split uploads all non-empty leaves before deleting the parent and stale blobs in its range. - Repair: recomputes a balanced layout from staged entries (splits and coarsens); stale-shard deletion unchanged. - Local store schema v2: entries keyed by content_hash BLOB ranges (no routed prefix column), recreate-on-version-mismatch, non-overlapping coverage claims. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lance Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…load Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ShardSerializer.Deserialize and SerializeAsync now require an ICompressionService argument (added by the zstd branch). ChunkIndexService and SnapshotService constructors also gained a compression parameter. Tests added by the dynamic-shard-prefix branch were written against the pre-zstd signatures; this commit threads TestCompression.Instance through all the affected call sites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughReplaces the fixed 2-hex-character chunk-index shard layout with a variable-depth dynamic scheme. ChangesDynamic chunk-index shard partitioning
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ChunkIndexService
participant ChunkIndexRouter
participant ChunkIndexLocalStore
participant BlobContainer
rect rgba(70, 130, 180, 0.5)
Note over Caller,BlobContainer: LookupAsync (root-gated)
Caller->>ChunkIndexService: LookupAsync(hashes)
ChunkIndexService->>ChunkIndexRouter: GetRootPrefix(hash)
ChunkIndexService->>ChunkIndexLocalStore: FindCoveredPrefixes(root, hashes, snapshot)
ChunkIndexService->>BlobContainer: ListAsync(chunk-index/root, BlobNamePrefix)
ChunkIndexService->>ChunkIndexRouter: ResolveTarget(existingShards, hash)
ChunkIndexService->>BlobContainer: DownloadAsync(shard blob)
ChunkIndexService->>ChunkIndexLocalStore: UpdatePrefix / AddEmptyPrefix
ChunkIndexService-->>Caller: hits map
end
rect rgba(60, 179, 113, 0.5)
Note over Caller,BlobContainer: FlushAsync (split-aware)
Caller->>ChunkIndexService: FlushAsync()
ChunkIndexService->>ChunkIndexLocalStore: GetRootsWithPendingFlushes()
ChunkIndexService->>ChunkIndexLocalStore: CountRangeEntries(prefix)
alt count <= MaxShardEntryCount
ChunkIndexService->>BlobContainer: UploadAsync(merged shard)
else count > MaxShardEntryCount
ChunkIndexService->>BlobContainer: UploadAsync(leaf child shards)
ChunkIndexService->>BlobContainer: DeleteAsync(parent + stale children)
end
ChunkIndexService->>ChunkIndexLocalStore: MarkPendingFlushesSynchronized(ETags)
ChunkIndexService-->>Caller: done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #111 +/- ##
==========================================
+ Coverage 80.77% 81.00% +0.22%
==========================================
Files 99 99
Lines 6340 6516 +176
Branches 852 886 +34
==========================================
+ Hits 5121 5278 +157
- Misses 986 1001 +15
- Partials 233 237 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs (1)
254-254:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve the unresolved TODO flagged by CI.
Line 254 is still carrying a
TODOmarker (S1135). Either implement the channel options now or replace this with a tracked issue reference to keep warnings actionable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs` at line 254, The Channel.CreateBounded call in the ArchiveCommandHandler constructor at line 254 has an unresolved TODO comment about specifying channel options like SingleWriter or MultipleReader. Either implement the specific BoundedChannelOptions with appropriate SingleReader, SingleWriter, and other relevant properties based on how the filePairChannel is used throughout the code, or replace the TODO comment with a tracked issue reference in the format of your tracking system (e.g., a Jira ticket, GitHub issue) to keep the warning actionable and avoid CI failures from unresolved TODOs.Source: Pipeline failures
🧹 Nitpick comments (1)
src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs (1)
18-22: TODO: Restore production threshold before merge.The constant is currently set to
16for testing, with the production value10_000noted in the TODO comment. This should be restored before the PR leaves draft status.Would you like me to create an issue to track restoring the production threshold?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs` around lines 18 - 22, The MaxShardEntryCount constant in the ChunkIndexService class is currently set to the test value of 16 instead of the production value of 10_000. Change the MaxShardEntryCount value from 16 to 10_000 and remove the TODO comment that indicates this is a temporary test value, ensuring the production threshold is restored before merging.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Arius.Core.Tests/Shared/Storage/FakeInMemoryBlobContainerServiceTests.cs`:
- Around line 46-76: Extract the PassthroughBlobContainerService class from the
test file and move it to a dedicated file in a local Fakes/ subfolder next to
the test file, following the coding guideline that scenario-specific test
doubles should be in a Fakes/ subfolder rather than nested within the test file
itself. Update the test class FakeInMemoryBlobContainerServiceTests to reference
the moved PassthroughBlobContainerService from its new location.
---
Outside diff comments:
In `@src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs`:
- Line 254: The Channel.CreateBounded call in the ArchiveCommandHandler
constructor at line 254 has an unresolved TODO comment about specifying channel
options like SingleWriter or MultipleReader. Either implement the specific
BoundedChannelOptions with appropriate SingleReader, SingleWriter, and other
relevant properties based on how the filePairChannel is used throughout the
code, or replace the TODO comment with a tracked issue reference in the format
of your tracking system (e.g., a Jira ticket, GitHub issue) to keep the warning
actionable and avoid CI failures from unresolved TODOs.
---
Nitpick comments:
In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs`:
- Around line 18-22: The MaxShardEntryCount constant in the ChunkIndexService
class is currently set to the test value of 16 instead of the production value
of 10_000. Change the MaxShardEntryCount value from 16 to 10_000 and remove the
TODO comment that indicates this is a temporary test value, ensuring the
production threshold is restored before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fbafec5-dbec-4362-93e2-f0173113f357
📒 Files selected for processing (41)
AGENTS.mdREADME.mddocs/cache.mdsrc/Arius.AzureBlob/AzureBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FakeMetadataOnlyBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FakeRecordingBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FaultingChunkIndexUploadBlobContainerService.cssrc/Arius.Core.Tests/Fakes/ThrowOnCreateBlobContainerService.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexLocalStoreTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexRouterTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceArchiveScenarioTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceFlushTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceLookupTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceRepairTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/BlobAlreadyExistsOnSetMetadataOnceBlobContainerService.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/BlockingDeleteBlobContainerService.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/ContentTypeCapturingBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/BlockingFileTreeUploadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/FaultingAndBlockingFileTreeUploadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/SlowDownloadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/ThrowingDownloadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/UnsortedSnapshotBlobContainerService.cssrc/Arius.Core.Tests/Shared/Storage/FakeInMemoryBlobContainerServiceTests.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexLocalStore.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexRouter.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/Shard.cssrc/Arius.Core/Shared/Storage/IBlobContainerService.cssrc/Arius.Explorer/Infrastructure/RepositorySession.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceArchiveScenarioTests.cssrc/Arius.Integration.Tests/ChunkIndex/Fakes/CountingBlobContainerService.cssrc/Arius.Integration.Tests/Pipeline/Fakes/FaultingBlobService.cssrc/Arius.Integration.Tests/Pipeline/Fakes/RehydrationSimulatingBlobService.cssrc/Arius.Tests.Shared/Hashes/HashTestData.cssrc/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs
💤 Files with no reviewable changes (1)
- src/Arius.Core/Shared/ChunkIndex/Shard.cs
Owner
Author
|
@CodeRabbit do a full review on this PR |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs (1)
19-19: ⚡ Quick winTODO needs resolution before release.
The
MaxShardEntryCount = 16is clearly a test value. The comment indicates it should be10_000for production. This will cause excessive shard splitting in production if not addressed.Do you want me to help track this as a separate issue, or is this intentionally set low for the current testing phase?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs` at line 19, The MaxShardEntryCount constant in ChunkIndexService.cs is set to a test value of 16 instead of the production value of 10_000 as indicated in the TODO comment. Change the MaxShardEntryCount constant from 16 to 10_000 and remove the TODO comment, as this test value will cause excessive shard splitting in production environments if left unresolved.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs`:
- Line 19: The MaxShardEntryCount constant in ChunkIndexService.cs is set to a
test value of 16 instead of the production value of 10_000 as indicated in the
TODO comment. Change the MaxShardEntryCount constant from 16 to 10_000 and
remove the TODO comment, as this test value will cause excessive shard splitting
in production environments if left unresolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51eda737-a443-4203-b313-6514bc0656d3
📒 Files selected for processing (41)
AGENTS.mdREADME.mddocs/cache.mdsrc/Arius.AzureBlob/AzureBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FakeMetadataOnlyBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FakeRecordingBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cssrc/Arius.Core.Tests/Fakes/FaultingChunkIndexUploadBlobContainerService.cssrc/Arius.Core.Tests/Fakes/ThrowOnCreateBlobContainerService.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexLocalStoreTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexRouterTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceArchiveScenarioTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceFlushTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceLookupTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceRepairTests.cssrc/Arius.Core.Tests/Shared/ChunkIndex/ShardTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/BlobAlreadyExistsOnSetMetadataOnceBlobContainerService.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/BlockingDeleteBlobContainerService.cssrc/Arius.Core.Tests/Shared/ChunkStorage/Fakes/ContentTypeCapturingBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/BlockingFileTreeUploadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/FaultingAndBlockingFileTreeUploadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/SlowDownloadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/ThrowingDownloadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/UnsortedSnapshotBlobContainerService.cssrc/Arius.Core.Tests/Shared/Storage/FakeInMemoryBlobContainerServiceTests.cssrc/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cssrc/Arius.Core/Features/ListQuery/ListQueryHandler.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexLocalStore.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexRouter.cssrc/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cssrc/Arius.Core/Shared/ChunkIndex/Shard.cssrc/Arius.Core/Shared/Storage/IBlobContainerService.cssrc/Arius.Explorer/Infrastructure/RepositorySession.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceArchiveScenarioTests.cssrc/Arius.Integration.Tests/ChunkIndex/Fakes/CountingBlobContainerService.cssrc/Arius.Integration.Tests/Pipeline/Fakes/FaultingBlobService.cssrc/Arius.Integration.Tests/Pipeline/Fakes/RehydrationSimulatingBlobService.cssrc/Arius.Tests.Shared/Hashes/HashTestData.cssrc/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs
💤 Files with no reviewable changes (1)
- src/Arius.Core/Shared/ChunkIndex/Shard.cs
…nst/Arius7 into dynamic-shard-prefix
This reverts commit f25265a.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests