[Prototype] FoundationDB#3481
Conversation
PR SummaryMedium Risk Overview Wires new SS config/CLI flags and TOML template fields for both backends, updates parsing/tests, and updates Introduces a Kafka-based offload consumer tool ( Reviewed by Cursor Bugbot for commit eff7ead. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eff7ead. Configure here.
| if s.cache == nil || value == nil || len(value) > maxHistoricalReadCacheValueBytes { | ||
| return | ||
| } | ||
| s.cache.Add(key, historicalReadCacheValue{value: bytes.Clone(value), found: true, valueKnown: true}) |
There was a problem hiding this comment.
Cache skips nil values causing repeated reader calls
Low Severity
cacheValue bails out when value == nil, so legitimate empty-value keys (non-deleted state with zero-length data) are never cached. Every subsequent Get for such a key bypasses the cache and hits the remote historical reader again. The nil check conflates "no value to cache" with "value is the empty byte slice," which are distinct states in MVCC stores.
Reviewed by Cursor Bugbot for commit eff7ead. Configure here.
| func scyllaHistoricalOffloadConfigured(cfg config.StateStoreConfig) bool { | ||
| return strings.TrimSpace(cfg.HistoricalOffloadScyllaHosts) != "" || | ||
| strings.TrimSpace(cfg.HistoricalOffloadScyllaKeyspace) != "" | ||
| } |
There was a problem hiding this comment.
Scylla offload detection too loose with OR check
Medium Severity
scyllaHistoricalOffloadConfigured returns true when either HistoricalOffloadScyllaHosts or HistoricalOffloadScyllaKeyspace is set. Setting only the keyspace (e.g., from a leftover example config) without hosts causes the node to fail at startup because NewScyllaReader validates that hosts are required. Worse, it also falsely conflicts with an enabled FoundationDB backend, producing a confusing "only one historical offload fallback" error. The FoundationDB check is stricter (single Enabled bool), creating an asymmetry.
Reviewed by Cursor Bugbot for commit eff7ead. Configure here.
| fdb.ApplyDefaults() | ||
| if err := fdb.Validate(); err != nil { | ||
| return fmt.Errorf("foundationdb: %w", err) | ||
| } |
There was a problem hiding this comment.
Consumer validates FoundationDB config on discarded copy
Low Severity
Validate() copies c.FoundationDB into a local variable, calls ApplyDefaults() on the copy, then validates the copy. The original c.FoundationDB remains unchanged (zero-valued fields intact). The Scylla path handles this differently — c.Scylla.Validate() internally creates its own copy. This asymmetry means post-Validate() inspection of c.FoundationDB sees un-defaulted values, which is fragile for any future code that reads the config after validation.
Reviewed by Cursor Bugbot for commit eff7ead. Configure here.


Describe your changes and provide context
Testing performed to validate your change