Skip to content

Commit 530c547

Browse files
petr-mullerpetr-muller
committed
1 parent 3ecb9f2 commit 530c547

1 file changed

Lines changed: 87 additions & 0 deletions

File tree

‎REVIEW.html‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
<!DOCTYPE html>
2+
<html lang="en">
3+
<head>
4+
<meta charset="UTF-8">
5+
<meta name="viewport" content="width=device-width, initial-scale=1.0">
6+
<title>PR #3409 Review: Cache Risk Analysis responses</title>
7+
<style>
8+
body { font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Helvetica, Arial, sans-serif; max-width: 860px; margin: 2rem auto; padding: 0 1rem; color: #1f2328; line-height: 1.6; }
9+
h1 { border-bottom: 1px solid #d1d9e0; padding-bottom: .5rem; }
10+
h2 { margin-top: 2rem; border-bottom: 1px solid #d1d9e0; padding-bottom: .3rem; }
11+
.issue { background: #fff8c5; border-left: 4px solid #d4a72c; padding: .75rem 1rem; margin: 1rem 0; border-radius: 4px; }
12+
.issue h3 { margin: 0 0 .5rem 0; color: #7a4f01; }
13+
.minor { background: #ddf4ff; border-left: 4px solid #54aeff; padding: .75rem 1rem; margin: 1rem 0; border-radius: 4px; }
14+
.minor h3 { margin: 0 0 .5rem 0; color: #0550ae; }
15+
.positive { background: #dafbe1; border-left: 4px solid #2da44e; padding: .75rem 1rem; margin: 1rem 0; border-radius: 4px; }
16+
.positive h3 { margin: 0 0 .5rem 0; color: #116329; }
17+
code { background: #f6f8fa; padding: .15rem .3rem; border-radius: 3px; font-size: 0.9em; }
18+
pre { background: #f6f8fa; padding: 1rem; border-radius: 6px; overflow-x: auto; }
19+
pre code { background: none; padding: 0; }
20+
.file { color: #656d76; font-size: 0.85em; }
21+
.verdict { background: #f6f8fa; border: 1px solid #d1d9e0; padding: 1rem; border-radius: 6px; margin-top: 2rem; }
22+
</style>
23+
</head>
24+
<body>
25+
26+
<h1>PR #3409: Cache Risk Analysis responses</h1>
27+
28+
<h2>Overview</h2>
29+
<p>Adds matview-aware Redis caching for test query results used by both Risk Analysis and the classic Sippy test API. Cache entries are invalidated when the underlying materialized view is refreshed, rather than relying solely on TTL expiration.</p>
30+
<p>Three-step approach:</p>
31+
<ol>
32+
<li>Record matview refresh timestamps in Redis after each refresh completes</li>
33+
<li>Store cached results wrapped with a timestamp</li>
34+
<li>On cache hit, compare timestamps to decide freshness</li>
35+
</ol>
36+
<p>Also removes the dead <code>/api/canary</code> endpoint and the HTTP-level <code>CacheTime</code> on <code>/api/tests</code> (replaced by the new application-level cache).</p>
37+
38+
<h2>Issues</h2>
39+
40+
<div class="issue">
41+
<h3>logrus.Fatalf on Redis connection refused</h3>
42+
<p class="file">pkg/api/cache.go:213</p>
43+
<pre><code>} else if strings.Contains(err.Error(), "connection refused") {
44+
logrus.WithError(err).Fatalf("redis URL specified but got connection refused; exiting due to cost issues in this configuration")
45+
}</code></pre>
46+
<p>This kills the entire process on a transient Redis connection failure during a single cache lookup. The comment says "cost issues" but this is a very aggressive response to what could be a momentary network blip. A <code>Fatal</code> in a cache helper called from HTTP request handlers is surprising and could cause an outage from a brief Redis hiccup.</p>
47+
<p>At minimum this deserves a clearer comment explaining the rationale. Better: a more targeted response (circuit breaker, or only fatal during startup probing).</p>
48+
</div>
49+
50+
<div class="issue">
51+
<h3>Wrong log message in BigQuery path</h3>
52+
<p class="file">pkg/api/tests.go, in <code>buildTestsResultsFromBigQuery</code> (diff line 824)</p>
53+
<pre><code>}).Debug("buildTestsResultsFromPostgres completed")</code></pre>
54+
<p>This is inside <code>buildTestsResultsFromBigQuery</code> but the log message says "Postgres". Copy-paste bug from the rename. Should say <code>"buildTestsResultsFromBigQuery completed"</code>.</p>
55+
</div>
56+
57+
<div class="minor">
58+
<h3>context.TODO() instead of real context</h3>
59+
<p class="file">pkg/api/tests.go, in <code>buildTestsResultsFromPostgres</code> (diff line 731)</p>
60+
<pre><code>result, errs := GetDataFromCacheOrMatview(context.TODO(), cacheClient, ...</code></pre>
61+
<p>The calling chain has a <code>context.Context</code> available (from the HTTP request in <code>PrintTestsJSONFromDB</code>, or from <code>variantsTestResultFunc</code>). Passing <code>context.TODO()</code> means cache operations won't respect request cancellation. Consider threading the context through <code>TestResultsSpec</code> or as a parameter.</p>
62+
</div>
63+
64+
<h2>Observations</h2>
65+
66+
<div class="positive">
67+
<h3>Thorough test coverage</h3>
68+
<p>Tests for <code>GetDataFromCacheOrMatview</code> cover: nil cache, miss, hit with/without refresh timestamp, invalidation after refresh, exact timestamp boundary, error result skipping, invalid refresh timestamp, different matviews invalidated independently, and cache duration propagation.</p>
69+
</div>
70+
71+
<div class="positive">
72+
<h3>Graceful degradation</h3>
73+
<p>When the matview refresh timestamp is missing from Redis, cached data is used until normal TTL expiry. When <code>cacheClient</code> is nil, the code falls through to direct DB queries. Good resilience design.</p>
74+
</div>
75+
76+
<div class="positive">
77+
<h3>Cleanup</h3>
78+
<p>Dead code removal (<code>PrintCanaryTestsFromDB</code>, <code>/api/canary</code> route) and replacing the HTTP-level <code>CacheTime</code> with proper application-level caching are welcome improvements. Confirmed no other callers exist for the removed code.</p>
79+
</div>
80+
81+
<div class="verdict">
82+
<h3>Verdict</h3>
83+
<p>Good PR. The matview-aware invalidation is a practical approach that avoids both stale data and unnecessary cache misses. The <code>Fatalf</code> on connection refused is the main concern worth discussing before merge. The BQ log message typo and <code>context.TODO()</code> are minor fixes.</p>
84+
</div>
85+
86+
</body>
87+
</html>

0 commit comments

Comments
 (0)