Pull updating commit hashes out of SnapshotWorker #56
Conversation
|
Warning Rate limit exceeded@myieye has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughReplaces array-based commit collections with SortedSet across repository, data-model, and snapshot layers; adds IComparable on CommitBase; introduces QueryHelpers ToSortedSet/ToSortedSetAsync and CrdtRepository helpers to compute/update commit parent hashes; updates signatures and call sites to use SortedSet. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Repo as CrdtRepository
participant DM as DataModel
participant SW as SnapshotWorker
Repo->>DM: GetCurrentSnapshotsAndPendingCommits()
DM-->>Repo: (snapshots, pendingCommits: SortedSet<Commit>)
note right of DM #E8F6EF: QueryHelpers.ToSortedSetAsync materializes SortedSet
DM->>SW: UpdateSnapshots(commitsToApply: SortedSet<Commit>)
note right of SW #FFF3E0: SnapshotWorker iterates commits in SortedSet order
SW->>SW: ApplyCommitChanges(commits: SortedSet<Commit>)
SW-->>DM: updated snapshots
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
myieye
force-pushed
the
pull-updating-commit-hashes-out-of-snapshot-worker
branch
from
0dc12e5 to
5167ce8
Compare
myieye
force-pushed
the
pull-updating-commit-hashes-out-of-snapshot-worker
branch
from
5167ce8 to
ed8b11b
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/SIL.Harmony.Core/QueryHelpers.cs (1)
68-76: Async helper LGTM; optional micro-tweakImplementation is correct. Optionally materialize first, then construct for slightly fewer Add operations on large sets.
- var set = new SortedSet<T>(); - await foreach (var item in queryable.AsAsyncEnumerable()) - { - set.Add(item); - } - return set; + var list = new List<T>(); + await foreach (var item in queryable.AsAsyncEnumerable()) list.Add(item); + return new SortedSet<T>(list);src/SIL.Harmony/Db/CrdtRepository.cs (3)
210-221: Be explicit when returning empty SortedSet
return (snapshots, []);depends on collection-expression conversion to SortedSet. Prefer explicit construction to avoid compiler/version pitfalls.- if (snapshots.Count == 0) return (snapshots, []); + if (snapshots.Count == 0) return (snapshots, new SortedSet<Commit>());
378-383: Clarify save semantics; avoid double SaveChanges (optional)
UpdateCommitHashes always saves; AddCommits(save: false) still persists changes (hashes). This diverges from prior “no save” expectations.
When save == true, you’ll Save twice (hash updates, then new commits). It works but adds overhead.
Document the new behavior, or thread a flag into UpdateCommitHashes to defer saving until after new commits are added, enabling one SaveChanges.
Alternatively, AddRange(newCommits) before UpdateCommitHashes, compute all hashes, then a single SaveChanges inside the surrounding transaction.
Please confirm this intentional save behavior (especially for the sync path using save:false).
Also applies to: 385-391
408-417: Consider skipping no-op hash rewrites (optional)If commit.ParentHash already matches previousCommitHash, skip SetParentHash to reduce dirty-tracking and the subsequent save work on large histories.
- foreach (var commit in commits) + foreach (var commit in commits) { - commit.SetParentHash(previousCommitHash); + if (commit.ParentHash != previousCommitHash) + commit.SetParentHash(previousCommitHash); previousCommitHash = commit.Hash; }src/SIL.Harmony/DataModel.cs (1)
197-218: Reduce IN-clause size by de-duplicating entityIdsUse Distinct() to avoid redundant parameters when building snapshotLookup for large batches.
- if (commitsToApply.Count > 10) + if (commitsToApply.Count > 10) { - var entityIds = commitsToApply.SelectMany(c => c.ChangeEntities.Select(ce => ce.EntityId)); + var entityIds = commitsToApply + .SelectMany(c => c.ChangeEntities.Select(ce => ce.EntityId)) + .Distinct(); snapshotLookup = await repo.CurrentSnapshots() .Where(s => entityIds.Contains(s.EntityId)) .Select(s => new KeyValuePair<Guid, Guid?>(s.EntityId, s.Id)) .ToDictionaryAsync(s => s.Key, s => s.Value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SIL.Harmony.Core/CommitBase.cs(2 hunks)src/SIL.Harmony.Core/QueryHelpers.cs(1 hunks)src/SIL.Harmony/DataModel.cs(8 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(4 hunks)src/SIL.Harmony/SnapshotWorker.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/SIL.Harmony/SnapshotWorker.cs (3)
src/SIL.Harmony.Core/QueryHelpers.cs (1)
SortedSet(63-66)src/SIL.Harmony/Commit.cs (3)
Commit(7-41)Commit(17-21)Commit(28-31)src/SIL.Harmony/Db/CrdtRepository.cs (16)
Task(18-21)Task(28-32)Task(33-37)Task(99-102)Task(107-110)Task(112-125)Task(127-136)Task(138-148)Task(152-155)Task(210-222)Task(224-227)Task(229-236)Task(238-246)Task(248-254)Task(256-263)Task(265-273)
src/SIL.Harmony.Core/QueryHelpers.cs (2)
src/SIL.Harmony.Core/CommitBase.cs (5)
CommitBase(10-54)CommitBase(20-23)CommitBase(57-68)CommitBase(59-61)CommitBase(63-65)src/SIL.Harmony/Db/CrdtRepository.cs (16)
Task(18-21)Task(28-32)Task(33-37)Task(99-102)Task(107-110)Task(112-125)Task(127-136)Task(138-148)Task(152-155)Task(210-222)Task(224-227)Task(229-236)Task(238-246)Task(248-254)Task(256-263)Task(265-273)
src/SIL.Harmony/DataModel.cs (2)
src/SIL.Harmony/Db/CrdtRepository.cs (20)
Task(18-21)Task(28-32)Task(33-37)Task(99-102)Task(107-110)Task(112-125)Task(127-136)Task(138-148)Task(152-155)Task(210-222)Task(224-227)Task(229-236)Task(238-246)Task(248-254)Task(256-263)Task(265-273)CrdtRepository(23-26)CrdtRepository(46-471)CrdtRepository(55-66)CrdtRepository(373-376)src/SIL.Harmony.Core/QueryHelpers.cs (1)
SortedSet(63-66)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
src/SIL.Harmony/DataModel.cs (14)
Task(60-67)Task(69-88)Task(91-100)Task(114-129)Task(144-175)Task(177-190)Task(197-218)Task(220-238)Task(240-249)Task(251-256)Task(258-261)Task(286-290)Task(292-295)Commit(102-112)src/SIL.Harmony.Core/QueryHelpers.cs (5)
Task(7-14)Task(16-23)Task(68-76)SortedSet(63-66)IEnumerable(86-92)src/SIL.Harmony/Commit.cs (4)
Commit(7-41)Commit(17-21)Commit(28-31)SetParentHash(23-27)
🔇 Additional comments (6)
src/SIL.Harmony.Core/CommitBase.cs (1)
10-10: Comparable implementation looks correctCompareTo aligns with DefaultOrder (DateTime, Counter, Id). Safe for SortedSet uniqueness.
Also applies to: 49-53
src/SIL.Harmony/SnapshotWorker.cs (2)
31-41: API shift to SortedSet is soundPassing SortedSet through keeps ordering consistent and removes redundant sorts.
Please confirm all call sites now supply SortedSet (DataModel, CrdtRepository) to avoid accidental re-sorting at boundaries.
52-55: ApplyCommitChanges iteration change looks goodStraight iteration over SortedSet is correct and consistent with CompareTo. Hash-updating concerns are removed from here as intended.
Also applies to: 62-109
src/SIL.Harmony/Db/CrdtRepository.cs (2)
392-406: Commits-to-apply computation is correctUnionBy on Id + ToSortedSet ensures full chain rewrite when inserting in the past. Nice.
329-329: Improved error context in AddSnapshotsIncluding the deleted-group flag in the message is helpful for triage. LGTM.
src/SIL.Harmony/DataModel.cs (1)
84-86: LGTM on SortedSet flow adoptionCall sites correctly adopt SortedSet and align with repository/worker changes.
Also applies to: 122-124, 245-249, 303-309, 339-340
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/SIL.Harmony.Core/QueryHelpers.cs (1)
63-67: Bug: ToSortedSet should explicitly construct SortedSetCollection expressions don’t reliably target-convert to SortedSet. Use explicit construction.
Apply:
- public static SortedSet<T> ToSortedSet<T>(this IEnumerable<T> queryable) where T : CommitBase - { - return [.. queryable]; - } + public static SortedSet<T> ToSortedSet<T>(this IEnumerable<T> queryable) where T : CommitBase + { + return new SortedSet<T>(queryable); + }src/SIL.Harmony/DataModel.cs (1)
159-161: Update stale comment about where hashes are updatedHashes are now computed/persisted in CrdtRepository.UpdateCommitHashes, not in SnapshotWorker.
Apply:
- //don't save since UpdateSnapshots will also modify newCommits with hashes, so changes will be saved once that's done + // Hashes are updated in CrdtRepository.UpdateCommitHashes; save:false avoids an extra Save here. + // Snapshots (if any) save remaining tracked changes afterward.
🧹 Nitpick comments (3)
src/SIL.Harmony.Core/CommitBase.cs (1)
10-10: Ordering looks good; consider adding non-generic IComparable for robustnessSortedSet will order via CompareKey as intended. To be extra safe across runtimes and generic variance, also implement non-generic IComparable.
Apply:
public abstract class CommitBase : IComparable<CommitBase> { @@ public int CompareTo(CommitBase? other) { if (other is null) return 1; return CompareKey.CompareTo(other.CompareKey); } + + // Optional: improves compatibility with APIs that use non-generic comparisons + public int CompareTo(object? obj) + { + return obj is CommitBase other ? CompareTo(other) : 1; + } }Also applies to: 49-53
src/SIL.Harmony/DataModel.cs (1)
197-218: Minor: avoid duplicate EntityId values in IN-clauseDistinct reduces SQL IN size for large batches.
Apply:
- var entityIds = commitsToApply.SelectMany(c => c.ChangeEntities.Select(ce => ce.EntityId)); + var entityIds = commitsToApply + .SelectMany(c => c.ChangeEntities.Select(ce => ce.EntityId)) + .Distinct();src/SIL.Harmony/Db/CrdtRepository.cs (1)
408-417: Save inside UpdateCommitHashes is intentional but double-saves may occurGiven the reordering above, this Save will also persist new commits when save=false. Keep in mind AddCommit/AddCommits(save:true) will Save again (benign). Consider documenting this behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SIL.Harmony.Core/CommitBase.cs(2 hunks)src/SIL.Harmony.Core/QueryHelpers.cs(1 hunks)src/SIL.Harmony/DataModel.cs(8 hunks)src/SIL.Harmony/Db/CrdtRepository.cs(4 hunks)src/SIL.Harmony/SnapshotWorker.cs(2 hunks)
🔇 Additional comments (13)
src/SIL.Harmony.Core/QueryHelpers.cs (1)
68-76: Async materialization to SortedSet is fineEnumerating and adding into a SortedSet ensures correct ordering. LGTM.
src/SIL.Harmony/SnapshotWorker.cs (3)
34-35: API shift to SortedSet is appropriateAccepting pre-sorted commits removes repeated ordering and simplifies call sites. LGTM.
Also applies to: 39-40
52-55: UpdateSnapshots signature change looks goodDelegating to ApplyCommitChanges with SortedSet keeps semantics intact. LGTM.
62-67: Iterating SortedSet ensures stable application orderUsing foreach over SortedSet avoids extra DefaultOrder calls. LGTM.
src/SIL.Harmony/DataModel.cs (5)
84-86: Good: consume repo.AddCommits and pass SortedSet onwardThe flow aligns with the new repository API. LGTM.
122-124: Good: single-commit path updated consistentlyConsistent with repository changes. LGTM.
245-249: Regeneration via ToSortedSetAsync is consistentMaterializing a SortedSet for replay is appropriate. LGTM.
303-309: Pending commits path matches new SnapshotWorker APIPassing SortedSet into ApplyCommitsToSnapshots keeps behavior. LGTM.
335-349: GetAtCommit replay path looks correctUsing ToSortedSetAsync and applying into a small snapshot map is efficient. LGTM.
src/SIL.Harmony/Db/CrdtRepository.cs (4)
210-222: OK: returning pending commits as SortedSetUsing ToSortedSetAsync ensures deterministic ordering for downstream. LGTM.
378-383: AddCommit path is fineReturns full set to apply; extra Save after AddNewCommits is acceptable.
385-390: AddCommits API looks goodsave flag preserved; returning updated set is helpful to avoid redundant queries. LGTM.
330-330: Improved error detail on snapshot saveIncluding the deleted-group flag in the message helps triage. LGTM.
myieye
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/SIL.Harmony/Db/CrdtRepository.cs (2)
378-383: Remove redundant SaveChangesAsync in AddCommit.AddNewCommits already persists updates. This extra SaveChanges is unnecessary work inside/alongside a transaction.
public async Task<SortedSet<Commit>> AddCommit(Commit commit) { - var updatedCommits = await AddNewCommits([commit]); - await _dbContext.SaveChangesAsync(); - return updatedCommits; + var updatedCommits = await AddNewCommits([commit]); + return updatedCommits; }
392-407: Materialize newCommits once to avoid multiple enumeration.newCommits is enumerated several times (Any, MinBy, UnionBy, AddRange). Materialize once; also use the array in AddRange/UnionBy.
private async Task<SortedSet<Commit>> AddNewCommits(IEnumerable<Commit> newCommits) { - if (newCommits is null || !newCommits.Any()) return []; - var oldestAddedCommit = newCommits.MinBy(c => c.CompareKey) + if (newCommits is null) return []; + var newCommitArray = newCommits as Commit[] ?? newCommits.ToArray(); + if (newCommitArray.Length == 0) return []; + var oldestAddedCommit = newCommitArray.MinBy(c => c.CompareKey) ?? throw new ArgumentException("Couldn't find oldest commit", nameof(newCommits)); var parentCommit = await FindPreviousCommit(oldestAddedCommit); var existingCommitsToUpdate = await GetCommitsAfter(parentCommit); var commitsToApply = existingCommitsToUpdate - .UnionBy(newCommits, c => c.Id) + .UnionBy(newCommitArray, c => c.Id) .ToSortedSet(); //we're inserting commits in the past/rewriting history, so we need to update the previous commit hashes await UpdateCommitHashes(commitsToApply, parentCommit); - _dbContext.AddRange(newCommits); + _dbContext.AddRange(newCommitArray); await _dbContext.SaveChangesAsync(); return commitsToApply; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SIL.Harmony/Db/CrdtRepository.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
src/SIL.Harmony/DataModel.cs (14)
Task(60-67)Task(69-88)Task(91-100)Task(114-129)Task(144-175)Task(177-190)Task(197-218)Task(220-238)Task(240-249)Task(251-256)Task(258-261)Task(286-290)Task(292-295)Commit(102-112)src/SIL.Harmony.Core/QueryHelpers.cs (5)
Task(7-14)Task(16-23)Task(68-76)SortedSet(63-66)IEnumerable(86-92)src/SIL.Harmony/Commit.cs (4)
Commit(7-41)Commit(17-21)Commit(28-31)SetParentHash(23-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
329-331: Improved error context on snapshot save.Including deleted:{grouping.Key} in the message helps triage. LGTM.
210-221: Comparer is correctly strictly total—code approved.The
CompareKeytuple(DateTimeOffset, long, Guid)uses C#'s built-in lexicographic tuple comparison (DateTime, then Counter, then Id). SinceGuidis unique, no two different commits can collapse as equal inSortedSet<Commit>. The concern is resolved.
409-417: Code is correct—no unique index on Commits.Hash, so the flagged collision risk does not apply.Verified that
CommitEntityConfighas onlyHasKey(c => c.Id)with no unique index on theHashproperty. The hypothetical concern in the review ("if there's a unique index") does not materialize. The implementation is sound:UpdateCommitHashesmodifies objects in-memory without saving, followed byAddRangeand a singleSaveChangesAsync, which is good separation and ordering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/SIL.Harmony/Db/CrdtRepository.cs (3)
210-221: Return type change to SortedSet and query shape look good; consider no-tracking for perf.These commits are read-only here. Add AsNoTracking() to reduce tracking overhead.
- var newCommits = await CurrentCommits() - .Include(c => c.ChangeEntities) + var newCommits = await CurrentCommits() + .AsNoTracking() + .Include(c => c.ChangeEntities) .WhereAfter(lastCommit) .ToSortedSetAsync();
378-383: DRY: Implement AddCommit via AddCommits to keep one persistence path.- public async Task<SortedSet<Commit>> AddCommit(Commit commit) - { - var updatedCommits = await AddNewCommits([commit]); - await _dbContext.SaveChangesAsync(); - return updatedCommits; - } + public Task<SortedSet<Commit>> AddCommit(Commit commit) + { + return AddCommits(new[] { commit }, save: true); + }
392-406: Avoid multiple enumeration; optionally track new commits before hash updates for clarity.- private async Task<SortedSet<Commit>> AddNewCommits(IEnumerable<Commit> newCommits) + private async Task<SortedSet<Commit>> AddNewCommits(IEnumerable<Commit> newCommits) { - if (newCommits is null || !newCommits.Any()) return []; - var oldestAddedCommit = newCommits.MinBy(c => c.CompareKey) + if (newCommits is null) return []; + var newCommitArray = newCommits as Commit[] ?? newCommits.ToArray(); + if (newCommitArray.Length == 0) return []; + var oldestAddedCommit = newCommitArray.MinBy(c => c.CompareKey) ?? throw new ArgumentException("Couldn't find oldest commit", nameof(newCommits)); var parentCommit = await FindPreviousCommit(oldestAddedCommit); var existingCommitsToUpdate = await GetCommitsAfter(parentCommit); var commitsToApply = existingCommitsToUpdate - .UnionBy(newCommits, c => c.Id) + .UnionBy(newCommitArray, c => c.Id) .ToSortedSet(); - //we're inserting commits in the past/rewriting history, so we need to update the previous commit hashes - UpdateCommitHashes(commitsToApply, parentCommit); - _dbContext.AddRange(newCommits); + // we're inserting commits in the past/rewriting history, so we need to update the previous commit hashes + UpdateCommitHashes(commitsToApply, parentCommit); + _dbContext.AddRange(newCommitArray); return commitsToApply; }Note: Reordering AddRange before UpdateCommitHashes is optional; functionally equivalent, but may read clearer since all entities are tracked before mutation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SIL.Harmony/Db/CrdtRepository.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/SIL.Harmony/Db/CrdtRepository.cs (5)
src/SIL.Harmony/DataModel.cs (14)
Task(60-67)Task(69-88)Task(91-100)Task(114-129)Task(144-175)Task(177-190)Task(197-218)Task(220-238)Task(240-249)Task(251-256)Task(258-261)Task(286-290)Task(292-295)Commit(102-112)src/SIL.Harmony.Core/QueryHelpers.cs (5)
Task(7-14)Task(16-23)Task(68-76)SortedSet(63-66)IEnumerable(86-92)src/SIL.Harmony/Commit.cs (4)
Commit(7-41)Commit(17-21)Commit(28-31)SetParentHash(23-27)src/SIL.Harmony/Db/ICrdtDbContext.cs (1)
AddRange(19-19)src/SIL.Harmony/Db/CrdtDbContextFactory.cs (1)
AddRange(87-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/SIL.Harmony/Db/CrdtRepository.cs (2)
329-331: More informative snapshot save failure message.Good improvement; keeps deleted/non-deleted batch context in logs.
408-416: Hash chain recompute is straightforward and correct.Good: starts from parent (or NullParentHash) and walks the sorted set.
Refactor to clean up some of #54
#54 introduced the possibility of a commit not generating any snapshots (by not generating a new snapshot if a change object was ignored, which happens in the edge case of a create-change being applied to an entity that already exists and is not deleted).
That change uncovered the fact, that we're relying on the call to
DbContext.SaveChangesAsync()inCrdtRepository.AddSnapshots()to persist updates to commit hashes.That's very subtle and is not a good separation of concerns.
This PR pulls that commit-hash-updating out of SnapshotWorker and does it immediately when commits are added to the db-context.
I also started making use of
SortedSet<Commit>, so that collections of commits can be passed around without each method having to make sure they're sorted.Summary by CodeRabbit