feat(rpc): add neutral trace cache primitives - #4065
danielntmd wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4065 +/- ##
==========================================
+ Coverage 79.19% 79.40% +0.21%
==========================================
Files 464 469 +5
Lines 35741 36147 +406
==========================================
+ Hits 28305 28703 +398
- Misses 7427 7435 +8
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review in progress
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
Overall, it looks good, some nitpicks here and there.
I will need to read the follow up PR to see the cache in action and give a proper review to this one.
|
|
||
| // Cache stores immutable values with one active owner per key. | ||
| type Cache[K comparable, V any] struct { | ||
| mu sync.Mutex |
There was a problem hiding this comment.
Now the mutex seems to lock everyone independently if they are readers or writers. I see that Lease only wants to read, so there is a maybe a benefit for it.
| // Publish stores read-only data and releases ownership. Released leases are ignored. | ||
| func (w *Lease[K, V]) Publish(value V) { | ||
| cache := w.cache | ||
| cache.mu.Lock() | ||
| defer cache.mu.Unlock() | ||
| if cache.flights[w.key] != w.flight { | ||
| return | ||
| } | ||
| cache.records.Add(w.key, value) | ||
| cache.finishLocked(&w.key, w.flight) | ||
| } | ||
|
|
||
| // Abort releases ownership without changing the value. Repeated calls are safe. | ||
| func (w *Lease[K, V]) Abort() { | ||
| cache := w.cache | ||
| cache.mu.Lock() | ||
| defer cache.mu.Unlock() | ||
| if cache.flights[w.key] != w.flight { | ||
| return | ||
| } | ||
| cache.finishLocked(&w.key, w.flight) | ||
| } |
There was a problem hiding this comment.
On the topic of mutexes, I am not sure if Lease interacting directly with Cache internal types is the best design, but honestly finding it hard to think of a better way to model this.
Just a comment, not required change needs to be associated with it
|
Claude finished @danielntmd's task in 16m 35s —— View job Review completeScope: Correctness/concurrency: Nits:
No blockers. The design (single in-flight owner per key, eviction independent of active leases, non-mutating cache reads) is sound and well exercised by the |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
d0024d7 to
f7cda0b
Compare
|
Claude finished @danielntmd's task in 9m 56s —— View job Review completeScope:
This revision already resolves every open item from the earlier review rounds:
Design: single in-flight owner per key, non-mutating cache reads, eviction independent of active leases — sound and well covered by tests for publication, replacement, eviction-during-ownership, cancellation, and retry-after-abort. Nit (already acknowledged, no action needed for this PR): a No blockers. Nothing new to add beyond what's already been addressed in this thread. |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Claude finished @danielntmd's task in 15m 6s —— View job Review completeScope: Checks performed:
Findings: none new. This PR has already been through several review rounds (rodrodros + automated bots), and every substantive item was addressed in the current revision:
Design review ( One design tradeoff, already raised by the automated reviewer and acknowledged by the author: a No blockers, no nits to add beyond what's already been resolved in this thread. |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
A lot better description, it can still be better.
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
Final comment on style, I will delve into functionality on the next review
|
Claude finished @danielntmd's task in 3m 11s —— View job Review completeScope:
Checks performed: What changed since the last full review round ( Correctness/concurrency: Known, accepted tradeoff (unchanged from prior rounds, no action needed): a No blockers, no nits. This revision only mechanically applies already-agreed-upon review feedback and does not introduce new issues. |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| kind cacheLookupKind | ||
| value V | ||
| done <-chan struct{} | ||
| work *Lease[K, V] |
There was a problem hiding this comment.
I understand that you mean this Lease is doing the work. lease might be a better field name
| if found && (accepts == nil || accepts(value)) { | ||
| return cacheLookup[K, V]{kind: cacheHit, value: value} | ||
| } | ||
| if flight, found := c.flights[*key]; found { | ||
| return cacheLookup[K, V]{kind: cacheWait, done: flight} | ||
| } | ||
| flight := make(chan struct{}) | ||
| c.flights[*key] = flight | ||
| return cacheLookup[K, V]{ | ||
| kind: cacheLease, | ||
| value: value, | ||
| work: &Lease[K, V]{cache: c, key: *key, flight: flight}, | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: kind looks a bit redundant since it can be inferred form the other values.
It could be set on runtime by doing a call to Kind() function by verifying the values, or it can be skipped altogether by having boolean getters like "IsLease", "IsHit" or "IsWait".
There was a problem hiding this comment.
Nit: it seems this file could be merged into result.go
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
Looks good, check before merging that we don't have any API just to support RPC v8
2 participants
User description
User description
User description
This PR introduces the foundation for sharing immutable, read-only block traces across RPC versions.
Retained Behavior:
Cache Behavior: