sql: SHOW RANGES does not work with offline node

[erikgrinaker]

This works in 23.1 but not on master. To reproduce:

roachprod create local -n 3
roachprod start local
cockroach workload init kv --splits 1000
roachprod sql local:1

> show ranges from database kv with details;

roachprod stop local:3

> show ranges from database kv with details;
ERROR: failed to dial into node 3 (NODE_STATUS_UNAVAILABLE): initial connection heartbeat failed: grpc: connection error: desc = "transport: error while dialing: dial tcp 10.166.0.3:26261: connect: connection refused" [code 14/Unavailable]

Jira issue: CRDB-29397
Epic: CRDB-30635

[blathers-crl]

Hi @erikgrinaker, please add branch-* labels to identify which branch(es) this release-blocker affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[erikgrinaker]

This is fallout from #103128, reverting it fixes the problem. It does work when I omit WITH DETAILS, and I get the same error with tenant_span_stats():

> select * from crdb_internal.tenant_span_stats(1);                                                                                                                                           
ERROR: failed to dial into node 3 (NODE_STATUS_DEAD): initial connection heartbeat failed: grpc: connection error: desc = "transport: error while dialing: dial tcp 10.166.0.3:26261: connect: connection refused" [code 14/Unavailable]

I suppose the root problem here is in tenant_span_stats() rather than SHOW RANGES.

[erikgrinaker]

We should also have a test for this. SHOW RANGES needs to work when nodes are offline.

[zachlite]

@erikgrinaker thanks for filing. SHOW RANGES... will work fine on 23.1, because the work integrating span stats with show ranges (#103128) was not backported.

[knz]

I suppose the root problem here is in tenant_span_stats() rather than SHOW RANGES.

I feel like an acceptable behavior here is to configure a timeout in each node request during the fanout, and report "unknown stats" when a node cannot be reached.

[erikgrinaker]

That seems brittle? Why do we have to reach all cluster nodes, as long as we have valid leases for all ranges somewhere?

[knz]

Why do we have to reach all cluster nodes, as long as we have valid leases for all ranges somewhere?

It's a good question. There's room for optimization here.

Current algorithm (pseudo-code)

1. list the nodes that have at least one replica that overlaps with the target span(s) (nodeIDsForSpan)
2. on each node:
   1. call getLocalStats with the list of spans known to have a replica on this node
   2. for each span:
      • for those ranges that are fully contained in a span, use a KV batch containing a collection of RangeStatsRequest via distsender (which is expected to hit locally) - flushBatchedContainedKeys
      • for those ranges that overlap partially (first and last range), call storage.ComputeStats which is a local call.

Areas for improvement

In decreasing order of impact:

• all this fan-out complexity is because storage.ComputeStats is a local call. If there was a KV request type that could handle ComputeStats, DistSender could own the entire networking here and the entire algorithm could collapse into a very small function.
• it seems strange we hit all nodes with at least one replica. Should we not restrict the fanout to the leaseholders only?
• we only use the distsender fast path inside the inner loop. The case of fully-included ranges should be hoisted at the top of the outer loop instead. DistSender can own the reaching out to leaseholder over the network.
• the KV batch constructed by flushBatchedContainedKeys may grow to become arbitrary large. This should be cut up in smaller pieces (maybe 100 ranges at a time).

[knz]

For the very first point, Zach would probably need some support from the KV team.
In the past, Zach was working within the confine of existing code patterns for observability functions. Doing this optimization is really a KV API extension, which is a somewhat different design direction.

@erikgrinaker who in the team would be best to brainstorm here?

[erikgrinaker]

@shralex can assign this.

[zachlite]

Is there value in a short term solution where a request to an offline node just returns an empty SpanStatsResponse, instead of invalidating the entire fan-out?

[knz]

Is there value in a short term solution where a request to an offline node just returns an empty SpanStatsResponse, instead of invalidating the entire fan-out?

Yes, that would help a little bit.

[knz]

Actually, regardless of the architecture you choose (current or with optimizations discussed above) there's still a path through KV that can time out. I believe in both case it is useful to have a customizable time out to retrieve partial data.