fix: skip an unusable client-route row, keep the refresh (DRIVER-201) - #1061
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant RouteUpdateEvent
participant ClientRoutesTopologyMonitor
participant AdminQuery
participant RouteCache
RouteUpdateEvent->>ClientRoutesTopologyMonitor: provide connection IDs
ClientRoutesTopologyMonitor->>ClientRoutesTopologyMonitor: filter configured IDs
ClientRoutesTopologyMonitor->>AdminQuery: query configured route rows
AdminQuery->>ClientRoutesTopologyMonitor: return route rows
ClientRoutesTopologyMonitor->>RouteCache: retain or evict routes
Suggested reviewers: Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to The refresh changes include coverage for malformed rows, cache retention, scoped event queries, and carry-over tracking. No unresolved merge-blocking issue is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🟡 Changes recommended
Empty addresses are rejected before a valid configured address override can be applied.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Ensures malformed client-route rows do not abort an entire topology refresh.
Changes:
- Validates empty addresses and invalid ports.
- Isolates constructor failures per row.
- Adds regression tests preserving valid routes.
File summaries
| File | Description |
|---|---|
ClientRoutesTopologyMonitor.java |
Skips unusable rows while continuing refreshes. |
ClientRoutesTopologyMonitorTest.java |
Tests mixed invalid and valid rows. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
070b4e2 to
7839f72
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Mixed full refreshes and targeted null-address refreshes can still remove valid cached routes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java:444
- A mixed full refresh still drops cached routes for every skipped row: as soon as one good row makes
newRoutesnon-empty, this replacement removes the old entry for each ID inskippedHostIds. That sends those nodes back to their private addresses, the same failure the targeted/all-unusable branches explicitly avoid. Preserve cached entries for skipped IDs while replacing valid and genuinely absent rows, and cover a pre-populated cache with one good and one unusable result.
} else {
consecutiveEmptyResults.set(0);
resolvedRoutesCache.set(Collections.unmodifiableMap(newRoutes));
LOG.debug(
"[{}] Updated client routes: {} routes loaded", logPrefix, newRoutes.size());
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
7839f72 to
dd967ce
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The all-unusable path retains cached routes whose host IDs are absent from a full refresh.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
dd967ce to
3e7d55e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Targeted refreshes can still evict routes for unreadable host IDs, and malformed addresses can prevent valid overrides.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java:470
- The table address is decoded before looking up the override. If
getString("address")throws for a malformed cell, the row is skipped even when itsconnection_idhas a valid configured override, contradicting the method's documented outright-replacement behavior. Resolve the override first and only read the table address as the fallback.
String tableAddress = row.isNull("address") ? null : row.getString("address");
String connId =
row.contains("connection_id") && !row.isNull("connection_id")
? row.getString("connection_id")
: null;
String override = connId == null ? null : connectionAddrOverrides.get(connId);
return override != null ? override : tableAddress;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
3e7d55e to
513991a
Compare
| } | ||
| Map<UUID, ClientRouteRecord> merged = new HashMap<>(newRoutes); | ||
| for (Map.Entry<UUID, ClientRouteRecord> entry : cached.entrySet()) { | ||
| if (!canProveAbsence || hostIdsInResult.contains(entry.getKey())) { |
There was a problem hiding this comment.
[P2] Retain by the complete route identity here. system.client_routes is keyed by (connection_id, host_id), but this check matches only host_id. With cached (A, H) and a refresh containing only an unusable (B, H) row (for example, a missing port), the deleted A route is retained forever because every nonempty pass resets the empty-result counter. Track the cached route source, or avoid carry-over when multiple connection IDs make the match ambiguous.
There was a problem hiding this comment.
Took the second option: carry-over now applies only where host_id is the whole route identity — one configured connection_id. With several, a refresh keeps only what it rebuilt (full refresh replaces, targeted sweep evicts), so the deleted A route goes on the next pass. The sweep keyed on hostIdsInResult too, so both writers now read one derived set.
Tracking the source doesn't stop at the record: once the key is (connection_id, host_id), the sweep needs a scope guard it doesn't need today — cached (A,H), an event naming only B, query connection_id IN ('B'), and absence of (A,H) from a result that never asked about A would evict a live route. Eviction scope then has to mirror the WHERE clause. That is #1063, which already owes a tie-break rule for the same reason, so I left it there.
Cost, stated in the javadoc: with several connection IDs an unusable row costs the route again — no worse than scylla-4.x, and in that config the cached entry is already whichever row the server returned last.
b440055080; 3 new tests, all red on the old head.
There was a problem hiding this comment.
Reversing the answer I gave here. The carve-out is gone in 455a0daadf, and hostIdIdentifiesRoute with it: retention no longer varies with the connection-ID count.
Your objection was that the cached route is retained forever, since every non-empty pass resets the empty-result counter. That is right, but dropping the route was the wrong way to bound it. An unusable row is evidence the route exists — a deletion arrives as an absent row, and absence still evicts — so the ambiguity you named is about which route the cached entry is, not about whether one is there. Dropping it costs the node its only reachable address until the table changes; keeping a stale one costs a connect that fails fast. So the unboundedness is now answered by reporting rather than by eviction: recordCarryOvers (:806) counts the passes that carry each route over and logs at ERROR past the third.
I also owe a correction on "no worse than scylla-4.x". Against ce317fa90c:
- the targeted sweep evicted on
!newRoutes.containsKey(hostId), so an event-named host whose row came back unusable was already evicted — unchanged by this PR, and the claim held there; - the full refresh did
resolvedRoutesCache.set(newRoutes)outright, so a skipped row's host was already dropped; - but the empty-result guard keyed on
newRoutes.isEmpty(), so an all-unusable pass bought three passes of grace. Keying it onrowCountremoved that, and the carve-out then emptied the cache in one pass with several connection IDs — or in none at all pre-PR, since a throwing row aborted the whole pass. That part of the claim was wrong, and it is what455a0daadffixes.
The ambiguity itself stays with #1063, where the cache key is.
Tests: the three multi-endpoint tests inverted, plus five covering the carry-over counting and the guard. 3989 core green; each guard re-checked by mutating it and watching the matching test fail.
513991a to
b440055
Compare
| Collectors.toMap( | ||
| ClientRouteProxy::getConnectionId, | ||
| ClientRouteProxy::getConnectionAddrOverride))); | ||
| this.hostIdIdentifiesRoute = new HashSet<>(configuredConnectionIds).size() == 1; |
There was a problem hiding this comment.
[P2] Derive route identity from IDs used by this query. Scylla broadcasts all changed keys, and buildQuery uses event IDs verbatim. With configured A plus unconfigured B, an unusable (B,H) row can make this flag retain a deleted cached (A,H) route. Filter event IDs against configured IDs or base this decision on actual query scope.
There was a problem hiding this comment.
Took the first remedy. allowedConnectionIds (ClientRoutesTopologyMonitor:594) intersects the event's IDs with the configured ones, and an event naming none of them is dropped rather than queried, so a pass's scope is always inside the configured set.
Splitting the two halves by provenance, since I ran them together before and that blurred it:
- The query taking event IDs verbatim is pre-existing, since
2dea0bacb1, the original PrivateLink commit —git log -S"eventConnectionIds"has nothing between. With it, a usable(B,H)row for an unconfigured proxy installs a route through a proxy this client is not configured to use. That consequence is mine, not something you raised; I previously wrote it as though it were yours. - The retention consequence is the one you named, and it was introduced by this PR: only
hostIdIdentifiesRoute(b440055080) made an unusable(B,H)row able to keep a deleted cached(A,H).
gocql has never done otherwise: filterAllowedConnectionIDs (client_routes.go:455-466) intersects and continues when the result is empty, and 8995b14 "always block unknown endpoints" removed the option to skip it. So the Java side was a port gap rather than a choice.
Since then hostIdIdentifiesRoute has gone (455a0daadf) — retention no longer varies with the connection-ID count, so the flag had no readers left. The filter stands on its own regardless: a row is only evidence about the connection it belongs to, and querying another tenant's proxy is wrong whatever retention does with the result.
An event naming no connection at all still falls back to every configured ID — that one carries no scope, so it cannot rule this session out.
Tests: should_query_only_the_configured_connection_ids_an_event_names and should_ignore_an_event_that_names_no_configured_connection_id. Both red against b440055080, where the query is literally WHERE connection_id IN ('conn-1', 'conn-unconfigured') AND host_id IN (...).
| unattributableRows, | ||
| rowCount); | ||
| } | ||
| return cachedRoutes.keySet(); |
There was a problem hiding this comment.
[P2] Include freshly rebuilt hosts in this set. During a targeted refresh with usable H1 plus an unreadable-ID row, an empty old cache makes this return empty; H1 is merged, then immediately removed by the event-host sweep. Union cached keys with newRoutes.keySet() (or otherwise preserve proven-present hosts).
There was a problem hiding this comment.
Fixed in f5a8c1803f, one line, now at ClientRoutesTopologyMonitor:783:
keepable.addAll(newRoutes.keySet());keepableHostIds used to return cachedRoutes.keySet() once a row had an unreadable host_id. Every host the pass rebuilt is now in the keep-set by construction. As of 455a0daadf that union is unconditional, below both branches, so a future branch cannot miss it.
Worth being explicit about where the defect came from: the shared keep-set is new in this PR (b440055080), so this is one the PR introduced rather than one it inherited. Before it, the sweep keyed on !newRoutes.containsKey(hostId), and a host the pass had just rebuilt was in newRoutes and so never swept.
The cause is that the set has two readers and I gave it one contract. withRetainedCachedRoutes starts from newRoutes and reads the set as what to add, so omitting a rebuilt host is harmless there. The targeted sweep reads it as the complete keep-list and removes every event host ID outside it, so omitting one deletes it. The union makes both readings the same set. The full-refresh writer is unaffected, as you say.
One correction to the framing: the trigger is not the empty cache but whether the rebuilt host was already cached. With cache {B}, result [good A, unreadable row] and an event naming both, A was merged and swept out too — so it bit every pass that first discovered a host.
Tests: should_keep_rebuilt_route_when_a_row_had_an_unreadable_host_id_and_cache_was_empty and should_keep_both_a_rebuilt_and_a_carried_over_route_when_a_row_was_unreadable, red against b440055080 with {} and {carried} respectively. Re-checked since by making the union a no-op: those two are exactly what fails.
ClientRouteRecord's constructor threw inside the row loop, so one bad system.client_routes row discarded every route in the pass. Catch per row, and build the query inside that try: a malformed host_id escaped holding the in-flight slot, stalling every later refresh. A skipped row is not a deleted row, so evict only where the pass can prove a host absent, count the passes carrying a route over, and report at ERROR from the third. Query every configured connection_id: a cached record names none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
da800c9 to
cd54a60
Compare
One unusable
system.client_routesrow took the whole refresh down:ClientRouteRecord's constructor threw inside the row loop, discarding every route in that pass.Fixes
host_idor port cell costs one route, not the pass.try: a malformedhost_idoff the wire escaped holding the in-flight slot, stalling every later route refresh and the node-list refresh chained onto it (pre-existing since2dea0bacb1).connection_addroverride before theaddresscolumn, so an empty, absent or undecodable column cannot defeat it.connection_id— Scylla broadcasts every changed key, so events routinely name other tenants' proxies and their rows drew our conclusions (same commit). Otherwise query every configured ID: a cached record names no connection, so absence is proof only once each has been asked.Changes, and what they cost
keepableHostIds.ERRORfrom the third, with per-host counts.Verified:
mvn clean test -pl core— 4233 tests, 0 failures; guards checked by mutation. Not covered: whether the server writes a permanently unusable row.Filed, not fixed: #1063 (last-write-wins per
host_id), #1064 (addressnever syntax-checked).Part of the #890 split. Refs: #890
Fixes DRIVER-1060