GH-94: Remove example configurations and refactor metadata caching logic in query engine and WAL reader#96
Merged
Merged
Conversation
WalkthroughAdds Parquet metadata caching, threads WAL query settings through the query provider chain, implements a Parquet-metadata-driven Loki label/value scanner (bypassing DataFusion), introduces WAL-related engine/Helm values and Prometheus/dashboard updates, and several API/constructor changes to propagate WAL and metadata-cache settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LokiExecutor as Loki Executor
participant LogMetadataScan as Log Metadata Scan
participant IcebergTable as Iceberg Table
participant ParquetReader as Parquet Reader
participant ObjectStore as Object Store
Client->>LokiExecutor: GET /labels or /label_values
LokiExecutor->>IcebergTable: load_logs_table()
LokiExecutor->>LogMetadataScan: scan_labels(table, selector) / scan_label_values(...)
LogMetadataScan->>IcebergTable: table.scan().with_filter(predicate).build().plan_files()
IcebergTable-->>LogMetadataScan: FileScanTasks
LogMetadataScan->>ParquetReader: open_file_direct / open_builder
ParquetReader->>ObjectStore: fetch footer/metadata or byte ranges (dictionaries)
ObjectStore-->>ParquetReader: bytes
ParquetReader-->>LogMetadataScan: metadata / decoded dictionary values
LogMetadataScan-->>LokiExecutor: BTreeSet<String> (labels or values)
LokiExecutor-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
crates/icegate-common/src/storage/cache.rs (1)
809-842: Redundant cache lookup within locked section.The cache is queried twice under the same lock: once at lines 809-814 to check coverage, and again at lines 831-837 to clone for gap computation. Since the lock prevents concurrent modifications, the second lookup is unnecessary—the entry from the first lookup can be reused.
♻️ Suggested optimization
// Re-check cache under lock — another task may have filled // the gaps while we waited. - if let Some(entry) = inner + let existing_entry = inner .cache .get(&key) .await - .map_err(|e| Error::new(ErrorKind::Unexpected, e.to_string()))? - { - let cached = entry.value(); + .map_err(|e| Error::new(ErrorKind::Unexpected, e.to_string()))?; + + if let Some(ref entry) = existing_entry { + let cached = entry.value(); if cached.covers(range_start, range_end) { // ... return early ... } } // Find uncached gaps while still under lock. - let existing: Option<CacheValue> = inner - .cache - .get(&key) - .await - .map_err(|e| Error::new(ErrorKind::Unexpected, e.to_string()))? - .map(|entry| entry.value().clone()); + let existing: Option<CacheValue> = existing_entry.map(|e| e.value().clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 809 - 842, The code does two inner.cache.get(&key).await lookups under the same lock; instead, perform the get once into a temporary (e.g., entry_opt), clone its value into a local (e.g., cached_clone) so you can reuse it for both the coverage check (where you currently use cached.read_range and make_read_response) and for computing gaps via find_gaps (populate existing from the cloned CacheValue instead of calling inner.cache.get a second time), removing the redundant second lookup while keeping the same logic around return on cache hit.crates/icegate-queue/src/reader.rs (1)
738-753: Concurrent misses can still stampede the object store.The cache lock is released before
read_parquet_metadata_from_store(), so two requests for the same segment can both miss, both do the HEAD/range reads, and only then race toput(). If this cache is meant to absorb query bursts, add per-key in-flight deduplication instead of only post-fetch insertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/reader.rs` around lines 738 - 753, Currently the cache lock is released before calling read_parquet_metadata_from_store, allowing concurrent misses to all fetch the same key; add per-key in-flight deduplication: introduce an in_flight map (e.g., inside the same metadata_cache wrapper or a new Mutex<HashMap<CacheKey, OneshootSender/Notify/SharedFuture>>) keyed by cache_key; on lookup, if missing insert a placeholder/future and release the lock, perform read_parquet_metadata_from_store(&cache_key, cancel_token).await, then put the result into metadata_cache via guard.put(cache_key, Arc::clone(&metadata)) and complete/notify the in_flight entry so waiters can receive the same Arc; if lookup finds an existing in_flight entry, await its completion and then return Arc::clone of the cached value; update code paths that reference metadata_cache, cache_key, read_parquet_metadata_from_store, guard.put, and Arc::clone accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/helm/icegate/values.yaml`:
- Around line 93-98: The chart change added walQueryEnabled: false which
silently reverts merged-provider behavior to Iceberg-only on upgrades; restore
the previous default behavior by setting walQueryEnabled to true (or remove the
key to preserve current behavior), and document this default in values.yaml
alongside walMetadataSizeHint so operators are informed; update any comments in
values.yaml referencing WAL behavior to reflect the preserved default and add a
breaking-change note in the chart changelog if you intentionally want to flip
the default.
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 3218-3226: The panels labeled by namespace/pod are still plotting
container-level series because the PromQL queries (targets "A" and "B") never
aggregate by (namespace, pod); fix this by wrapping the container-level rate
expressions in sum by(namespace, pod)(...) — specifically change target "A"
(panel 102) to use sum by(namespace, pod)(rate(...)) and for target "B" apply
the same aggregation after the uid join (i.e., perform the on(uid)
group_left(namespace, pod) join, then sum by(namespace, pod)(...) so each pod
yields a single series per namespace/pod).
- Around line 3642-3659: The legendFormat fields for the three load queries (the
panels with expr "node_load1 ...", "node_load5 ...", and "node_load15 ...",
refId values A, B, C) are static strings and must include the node label so each
series is identifiable; update each legendFormat to include the instance/node
label placeholder (e.g. use {{instance}} or {{node}} depending on which label
your metrics expose) such as "{{instance}} load1/core", "{{instance}}
load5/core", and "{{instance}} load15/core" so the per-node series are
distinguishable in multi-node clusters.
In `@config/kustomize/base/values-prometheus.yaml`:
- Around line 127-139: The two recording rules container:cpu_cfs_throttle_ratio
and container:cpu_cfs_throttle_ratio:by_uid must aggregate the per-container
series to pod scope before computing the ratio; modify each expr so the
numerator and denominator use sum(...) by (namespace, pod) (for the uid rule sum
by (uid) then * on(uid) group_left(namespace,pod) with topk join preserved) and
then divide the aggregated numerator by the aggregated denominator so each
resulting series is one per pod as expected.
- Around line 82-90: The regex under the rule that sets targetLabel uid is
written for cgroup v1 paths and therefore never matches cgroup v2 .slice paths,
causing uid to be empty and metrics to be dropped by the subsequent drop rule;
update the extraction logic so it handles systemd cgroup v2 .slice notation and
the cgroupsPerQOS=false case by replacing the current regex (the rule using
sourceLabels: [id], regex:
'/kubepods/(?:burstable|guaranteed|besteffort)/pod([^/]+)/.*', targetLabel: uid)
with one or more alternate regexes that match cgroup v2 .slice entries (e.g.
capture the pod<uid>.slice segment like
'kubepods(?:-[^.]*)?\.slice\/kubepods-[^.]*)?-pod([0-9a-fA-F-]+)\.slice' or
equivalent) and/or a regex for '/kubepods/pod([^/]+)/.*' for cgroupfs no-QoS;
ensure the drop rule (sourceLabels: [id, pod, uid], action: drop, regex: '.+;;')
only drops when uid truly empty and pod empty by keeping that check but first
reliably populating uid with the new regexes.
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 896-907: The race occurs between releasing the lock after Phase 3
and Phase 4's cache read: if inner.cache.get(&key).await returns None or a
non-covering entry, fall back to using the already-fetched data instead of
returning the `"cache merge failed to cover range"` error; modify the Phase 4
logic around inner.cache.get(&key).await /
entry.as_ref().and_then(...).ok_or_else(...) so that on cache miss or missing
range you construct the response from the existing fetched value (the variable
fetched/its buffer and total_size) and call make_read_response(fallback_data,
range_start, range_end, total_size) — alternatively you may choose to keep the
existing lock until after the read, but if opting for fallback ensure you
reference inner.cache.get, make_read_response, fetched and replace the error
path with the fallback construction.
In `@crates/icegate-query/src/engine/config.rs`:
- Around line 68-80: The config added a new field wal_query_enabled that
defaults to false, which silently disables the merged/WAL provider for callers
that don't set it; change the default to true so the engine preserves prior
semantics (merged provider enabled) by updating the Default/new implementation
or builder that initializes wal_query_enabled and any other place initializing
that field (refer to wal_query_enabled and wal_metadata_size_hint in config.rs,
and the other initialization around lines 91-92) to set wal_query_enabled = true
instead of false so callers must opt out explicitly.
In `@crates/icegate-query/src/engine/provider/table.rs`:
- Around line 200-203: Move the WAL-disabled guard from inside the later
processing to the start of scan(): before calling extract_wal_offset() check
self.wal_config.enabled and immediately return Ok(None) (and log the debug
message) when disabled so Iceberg-only queries do not run extract_wal_offset()
or traverse snapshot history; update the scan() implementation to perform this
early short-circuit and remove or keep the later redundant check in the code
path guarded by extract_wal_offset().
---
Nitpick comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 809-842: The code does two inner.cache.get(&key).await lookups
under the same lock; instead, perform the get once into a temporary (e.g.,
entry_opt), clone its value into a local (e.g., cached_clone) so you can reuse
it for both the coverage check (where you currently use cached.read_range and
make_read_response) and for computing gaps via find_gaps (populate existing from
the cloned CacheValue instead of calling inner.cache.get a second time),
removing the redundant second lookup while keeping the same logic around return
on cache hit.
In `@crates/icegate-queue/src/reader.rs`:
- Around line 738-753: Currently the cache lock is released before calling
read_parquet_metadata_from_store, allowing concurrent misses to all fetch the
same key; add per-key in-flight deduplication: introduce an in_flight map (e.g.,
inside the same metadata_cache wrapper or a new Mutex<HashMap<CacheKey,
OneshootSender/Notify/SharedFuture>>) keyed by cache_key; on lookup, if missing
insert a placeholder/future and release the lock, perform
read_parquet_metadata_from_store(&cache_key, cancel_token).await, then put the
result into metadata_cache via guard.put(cache_key, Arc::clone(&metadata)) and
complete/notify the in_flight entry so waiters can receive the same Arc; if
lookup finds an existing in_flight entry, await its completion and then return
Arc::clone of the cached value; update code paths that reference metadata_cache,
cache_key, read_parquet_metadata_from_store, guard.put, and Arc::clone
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8023e71-7c9a-4058-96e3-f21c165af7cc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Cargo.tomlconfig/examples/ingest.tomlconfig/examples/ingest.yamlconfig/examples/query.tomlconfig/examples/query.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/dashboards/Nodes-1769530183590.jsonconfig/kustomize/base/values-prometheus.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/prefetch.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/engine/config.rscrates/icegate-query/src/engine/core.rscrates/icegate-query/src/engine/provider/catalog.rscrates/icegate-query/src/engine/provider/mod.rscrates/icegate-query/src/engine/provider/schema.rscrates/icegate-query/src/engine/provider/table.rscrates/icegate-queue/Cargo.tomlcrates/icegate-queue/src/config.rscrates/icegate-queue/src/reader.rs
💤 Files with no reviewable changes (4)
- config/examples/query.yaml
- config/examples/ingest.toml
- config/examples/query.toml
- config/examples/ingest.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/icegate-common/src/storage/cache.rs (1)
867-909:⚠️ Potential issue | 🔴 CriticalFallback still drops bytes that were already cached before the fetch.
When this is a partial miss,
fetchedonly contains the gaps. If the cache entry disappears between Phase 3 and Phase 4, rebuilding fromfetchedalone can still fail for ranges that depended on the pre-existing cached segments, so the read remains spuriously error-prone.🩹 Keep a local fallback copy of the merged range before releasing Phase 3
- // Phase 3: Re-acquire lock briefly to merge fetched data. - { + // Phase 3: Re-acquire lock briefly to merge fetched data and keep + // a local fallback copy of the requested range. + let fallback_data = { let lock = inner.locks.lock(&key); let _guard = lock.lock().await; @@ let mut merged = existing.unwrap_or_else(CacheValue::new); for (offset, data) in &fetched { merged.insert_range(*offset, &data.to_bytes()); } + + let fallback_data = merged.read_range(range_start, range_end).ok_or_else(|| { + Error::new(ErrorKind::Unexpected, "cache merge failed to cover requested range") + })?; // Update cache. inner.cache.insert(key.clone(), merged); - } + fallback_data + }; @@ let data = match data { Some(d) => d, None => { - // Cache evicted between merge and read — reconstruct from - // the fetched buffers we still hold. - tracing::debug!("Cache miss after merge, using fetched data as fallback"); - let mut fallback = CacheValue::new(); - for (offset, buf) in &fetched { - fallback.insert_range(*offset, &buf.to_bytes()); - } - fallback.read_range(range_start, range_end).ok_or_else(|| { - Error::new(ErrorKind::Unexpected, "fetched data does not cover requested range") - })? + tracing::debug!("Cache miss after merge, using local fallback"); + fallback_data } };A regression test for a partial-hit + eviction-after-merge case would be useful here too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 867 - 909, The fallback rebuild currently uses only `fetched`, losing bytes that were present in the cache before Phase 3; capture the merged state while still holding the lock and use it as the primary fallback. Specifically, inside the Phase 3 block where `merged` is constructed (the code that uses `inner.locks.lock(&key)` and `existing` to build `merged`), clone or take a local copy of `merged` (e.g., `merged_fallback`) before releasing the lock, and then in Phase 4, if the cache read returns `None`, reconstruct the range by merging `merged_fallback` with `fetched` (apply `insert_range` from both sources) and then call `read_range(range_start, range_end)` on that combined fallback instead of using `fetched` alone; update names `merged`, `fetched`, `CacheValue`, `range_start`, and `range_end` accordingly and add a regression test for partial-hit + eviction-after-merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 867-909: The fallback rebuild currently uses only `fetched`,
losing bytes that were present in the cache before Phase 3; capture the merged
state while still holding the lock and use it as the primary fallback.
Specifically, inside the Phase 3 block where `merged` is constructed (the code
that uses `inner.locks.lock(&key)` and `existing` to build `merged`), clone or
take a local copy of `merged` (e.g., `merged_fallback`) before releasing the
lock, and then in Phase 4, if the cache read returns `None`, reconstruct the
range by merging `merged_fallback` with `fetched` (apply `insert_range` from
both sources) and then call `read_range(range_start, range_end)` on that
combined fallback instead of using `fetched` alone; update names `merged`,
`fetched`, `CacheValue`, `range_start`, and `range_end` accordingly and add a
regression test for partial-hit + eviction-after-merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a33a16bc-5f53-4911-87d7-5b1174c0e6ef
📒 Files selected for processing (4)
config/kustomize/base/dashboards/Nodes-1769530183590.jsonconfig/kustomize/base/values-prometheus.yamlcrates/icegate-common/src/storage/cache.rscrates/icegate-query/src/engine/provider/table.rs
✅ Files skipped from review due to trivial changes (1)
- config/kustomize/base/values-prometheus.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/icegate-query/src/engine/provider/table.rs
- config/kustomize/base/dashboards/Nodes-1769530183590.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/kustomize/base/dashboards/Nodes-1769530183590.json (2)
2441-2459:⚠️ Potential issue | 🟠 MajorKeep the usage and limit queries at the same pod granularity.
Target B is now pod-aggregated, but target A on Line 2441 still emits per-container memory usage. Multi-container pods will render several indistinguishable
{{pod}}usage lines against one total pod limit, so the panel is no longer comparing like-for-like.📏 Match both targets at pod scope
- "expr": "container_memory_usage_bytes{pod!=\"\"}", + "expr": "sum by(namespace, pod) (container_memory_usage_bytes{pod!=\"\"})", ... - "legendFormat": "{{pod}}", + "legendFormat": "{{namespace}}/{{pod}}", ... - "legendFormat": "Limit {{pod}}", + "legendFormat": "Limit {{namespace}}/{{pod}}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Nodes-1769530183590.json` around lines 2441 - 2459, The panel mixes per-container usage (target A: expr "container_memory_usage_bytes{pod!=\"\"}") with a pod-level limit (target B: sum by(namespace, pod) (...)), so change target A to aggregate usage to pod level—e.g., replace the container_memory_usage_bytes expression with a sum by(namespace, pod) wrapper so both targets use the same pod granularity; keep the legendFormat "{{pod}}" and other fields unchanged so the usage and Limit lines are comparable.
2864-2868:⚠️ Potential issue | 🟠 MajorPreserve
namespacewhen aggregating pod disk I/O.
sum by(pod)will mergenamespace-a/fooandnamespace-b/foointo one series. That makes these panels wrong on any multi-namespace cluster with repeated pod names.🧾 Keep namespace in the aggregation and legend
- "expr": "sum by(pod) (irate(container_fs_writes_bytes_total{pod!=\"\",device!~\"/dev/dm.*\"}[$__rate_interval]))", - "legendFormat": "{{pod}}", + "expr": "sum by(namespace, pod) (irate(container_fs_writes_bytes_total{pod!=\"\",device!~\"/dev/dm.*\"}[$__rate_interval]))", + "legendFormat": "{{namespace}}/{{pod}}", ... - "expr": "sum by(pod) (irate(container_fs_reads_bytes_total{pod!=\"\",device!~\"/dev/dm.*\"}[$__rate_interval]))", - "legendFormat": "{{pod}}", + "expr": "sum by(namespace, pod) (irate(container_fs_reads_bytes_total{pod!=\"\",device!~\"/dev/dm.*\"}[$__rate_interval]))", + "legendFormat": "{{namespace}}/{{pod}}",Also applies to: 2964-2968
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Nodes-1769530183590.json` around lines 2864 - 2868, The panel aggregates disk I/O using the expression "sum by(pod) (irate(container_fs_writes_bytes_total{...}))" which collapses identically-named pods from different namespaces; change the aggregation to include namespace (use sum by(namespace,pod)) and update the legendFormat from "{{pod}}" to include namespace (e.g. "{{namespace}}/{{pod}}") so series remain unique; apply the same change to the other similar panel that also uses container_fs_writes_bytes_total with sum by(pod).
♻️ Duplicate comments (1)
config/kustomize/base/dashboards/Nodes-1769530183590.json (1)
3328-3337:⚠️ Potential issue | 🟠 MajorPanel 102 still plots container-level series under pod legends.
Both targets label traces as
{{namespace}}/{{pod}}, but neither query aggregates to(namespace, pod), so multi-container pods will still render duplicate series with the same display name. This also diverges from the pod-scoped throttling rules inconfig/kustomize/base/values-prometheus.yaml:130-156.📉 Aggregate panel 102 to pod scope, like panel 101
- "expr": "rate(container_cpu_cfs_throttled_periods_total{pod!=\"\"}[5m])", + "expr": "sum by(namespace, pod) (rate(container_cpu_cfs_throttled_periods_total{pod!=\"\"}[5m]))", ... - "expr": "rate(container_cpu_cfs_throttled_periods_total{pod=\"\",uid!=\"\"}[5m])\n* on(uid) group_left(namespace, pod)\ntopk by(uid) (1, kube_pod_info)", + "expr": "sum by(namespace, pod) (\n rate(container_cpu_cfs_throttled_periods_total{pod=\"\",uid!=\"\"}[5m])\n * on(uid) group_left(namespace, pod)\n topk by(uid) (1, kube_pod_info)\n)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Nodes-1769530183590.json` around lines 3328 - 3337, Panel 102 is generating container-level series but labels them with pod-level legendFormat "{{namespace}}/{{pod}}"; update both Prometheus expressions that use container_cpu_cfs_throttled_periods_total so they aggregate to pod scope (namespace, pod) before plotting — e.g. replace the raw rate(...) and the rate(...)*on(uid) group_left(...) construction with an aggregation like sum by(namespace, pod) (rate(container_cpu_cfs_throttled_periods_total{...}[5m])) or an equivalent join that yields one series per (namespace, pod), ensuring the legendFormat matches the aggregated labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 2441-2459: The panel mixes per-container usage (target A: expr
"container_memory_usage_bytes{pod!=\"\"}") with a pod-level limit (target B: sum
by(namespace, pod) (...)), so change target A to aggregate usage to pod
level—e.g., replace the container_memory_usage_bytes expression with a sum
by(namespace, pod) wrapper so both targets use the same pod granularity; keep
the legendFormat "{{pod}}" and other fields unchanged so the usage and Limit
lines are comparable.
- Around line 2864-2868: The panel aggregates disk I/O using the expression "sum
by(pod) (irate(container_fs_writes_bytes_total{...}))" which collapses
identically-named pods from different namespaces; change the aggregation to
include namespace (use sum by(namespace,pod)) and update the legendFormat from
"{{pod}}" to include namespace (e.g. "{{namespace}}/{{pod}}") so series remain
unique; apply the same change to the other similar panel that also uses
container_fs_writes_bytes_total with sum by(pod).
---
Duplicate comments:
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 3328-3337: Panel 102 is generating container-level series but
labels them with pod-level legendFormat "{{namespace}}/{{pod}}"; update both
Prometheus expressions that use container_cpu_cfs_throttled_periods_total so
they aggregate to pod scope (namespace, pod) before plotting — e.g. replace the
raw rate(...) and the rate(...)*on(uid) group_left(...) construction with an
aggregation like sum by(namespace, pod)
(rate(container_cpu_cfs_throttled_periods_total{...}[5m])) or an equivalent join
that yields one series per (namespace, pod), ensuring the legendFormat matches
the aggregated labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7db59c92-cc28-4669-b7e0-04bdd27dbf6b
📒 Files selected for processing (1)
config/kustomize/base/dashboards/Nodes-1769530183590.json
…gic in query engine and WAL reader
…e cache handling logic, and introduce early exit for disabled WAL in query engine
…tency and accuracy
frisbeeman
force-pushed
the
GH-94/query-optimize
branch
from
b7b2120 to
850a446
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/kustomize/base/dashboards/Nodes-1769530183590.json (1)
2215-2235:⚠️ Potential issue | 🟠 MajorAvoid cross-node NIC conflation by grouping on
instance,device.Line 2215, Line 2666, and Line 2765 aggregate only by
device, which mergeseth0/ens*across nodes.🔧 Proposed fix
- "expr": "sum(rate(node_network_receive_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval])) by (device)", - "legendFormat": "received {{device}}", + "expr": "sum(rate(node_network_receive_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval])) by (instance, device)", + "legendFormat": "{{instance}} received {{device}}", ... - "expr": "- sum(rate(node_network_transmit_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval])) by (device)", - "legendFormat": "sent {{device}}", + "expr": "- sum(rate(node_network_transmit_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval])) by (instance, device)", + "legendFormat": "{{instance}} sent {{device}}", ... - "expr": "sum by(device) (rate(node_network_receive_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval]))", - "legendFormat": "{{device}}", + "expr": "sum by(instance, device) (rate(node_network_receive_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval]))", + "legendFormat": "{{instance}} {{device}}", ... - "expr": "sum by(device) (rate(node_network_transmit_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval]))", - "legendFormat": "{{device}}", + "expr": "sum by(instance, device) (rate(node_network_transmit_bytes_total{device!~\"lo|veth.*|cali.*|flannel.*\"}[$__rate_interval]))", + "legendFormat": "{{instance}} {{device}}",Also applies to: 2666-2669, 2765-2768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Nodes-1769530183590.json` around lines 2215 - 2235, The Prometheus queries currently aggregate network metrics only by device (e.g., the expr values using "sum(rate(node_network_receive_bytes_total...)) by (device)" and the transmit counterpart), which conflates identical NIC names across nodes; change the aggregation to group by both instance and device ("by (instance, device)") for the receive and transmit expressions (the entries with legendFormat "received {{device}}" and "sent {{device}}") and update the legendFormat to include the instance (e.g., "received {{instance}} {{device}}" / "sent {{instance}} {{device}}") so each series is per-node per-device; apply the same modification to the other two matching expr entries mentioned in the comment.
♻️ Duplicate comments (1)
config/kustomize/base/dashboards/Nodes-1769530183590.json (1)
3328-3337:⚠️ Potential issue | 🟠 MajorPanel 102 still returns container-level series under pod legends.
Line 3328 and Line 3336 still miss
sum by(namespace, pod), so multi-container pods will render duplicatenamespace/podlines.🔧 Proposed fix
- "expr": "rate(container_cpu_cfs_throttled_periods_total{pod!=\"\"}[5m])", + "expr": "sum by(namespace, pod) (rate(container_cpu_cfs_throttled_periods_total{pod!=\"\"}[5m]))", ... - "expr": "rate(container_cpu_cfs_throttled_periods_total{pod=\"\",uid!=\"\"}[5m])\n* on(uid) group_left(namespace, pod)\ntopk by(uid) (1, kube_pod_info)", + "expr": "sum by(namespace, pod) (\n rate(container_cpu_cfs_throttled_periods_total{pod=\"\",uid!=\"\"}[5m])\n * on(uid) group_left(namespace, pod)\n topk by(uid) (1, kube_pod_info)\n)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Nodes-1769530183590.json` around lines 3328 - 3337, Panel 102 uses raw container-level series which produce duplicate namespace/pod legends; update the PromQL expressions that reference container_cpu_cfs_throttled_periods_total and the join with kube_pod_info so the container series are aggregated per pod before the join (e.g., apply sum by(namespace, pod) or sum by(uid, namespace, pod) as appropriate) to ensure a single namespace/pod timeseries per pod and prevent multi-container pods from rendering duplicate lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 3438-3440: The ratio panels currently divide by sums of
kube_pod_container_resource_limits (CPU) and
kube_pod_container_resource_requests (CPU) and can produce +Inf/NaN when those
sums are zero or missing; update the PromQL expressions that compute the
denominators (the sum by(namespace, pod) of
kube_pod_container_resource_limits{resource="cpu"} and of
kube_pod_container_resource_requests{resource="cpu"}) to be guarded by clamp_min
(or wrapped with max(..., <small_nonzero>)) so the divisor is never zero (apply
the same change to both the limit and request ratio panels that use
container_cpu_usage_seconds_total divided by those sums).
---
Outside diff comments:
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 2215-2235: The Prometheus queries currently aggregate network
metrics only by device (e.g., the expr values using
"sum(rate(node_network_receive_bytes_total...)) by (device)" and the transmit
counterpart), which conflates identical NIC names across nodes; change the
aggregation to group by both instance and device ("by (instance, device)") for
the receive and transmit expressions (the entries with legendFormat "received
{{device}}" and "sent {{device}}") and update the legendFormat to include the
instance (e.g., "received {{instance}} {{device}}" / "sent {{instance}}
{{device}}") so each series is per-node per-device; apply the same modification
to the other two matching expr entries mentioned in the comment.
---
Duplicate comments:
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 3328-3337: Panel 102 uses raw container-level series which produce
duplicate namespace/pod legends; update the PromQL expressions that reference
container_cpu_cfs_throttled_periods_total and the join with kube_pod_info so the
container series are aggregated per pod before the join (e.g., apply sum
by(namespace, pod) or sum by(uid, namespace, pod) as appropriate) to ensure a
single namespace/pod timeseries per pod and prevent multi-container pods from
rendering duplicate lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f569f3c6-3088-410b-9cf3-e2896805ec57
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Cargo.tomlconfig/examples/ingest.tomlconfig/examples/ingest.yamlconfig/examples/query.tomlconfig/examples/query.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/dashboards/Nodes-1769530183590.jsonconfig/kustomize/base/values-prometheus.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/prefetch.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/engine/config.rscrates/icegate-query/src/engine/core.rscrates/icegate-query/src/engine/provider/catalog.rscrates/icegate-query/src/engine/provider/mod.rscrates/icegate-query/src/engine/provider/schema.rscrates/icegate-query/src/engine/provider/table.rscrates/icegate-queue/Cargo.tomlcrates/icegate-queue/src/config.rscrates/icegate-queue/src/reader.rs
💤 Files with no reviewable changes (4)
- config/examples/query.toml
- config/examples/ingest.yaml
- config/examples/ingest.toml
- config/examples/query.yaml
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- config/kustomize/overlays/skaffold/values-icegate.yaml
- config/helm/icegate/templates/configmap-query.yaml
- crates/icegate-queue/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/icegate-query/src/cli/commands/run.rs
- crates/icegate-query/src/engine/provider/mod.rs
- crates/icegate-common/src/storage/prefetch.rs
- config/helm/icegate/values.yaml
- crates/icegate-query/src/engine/provider/table.rs
- crates/icegate-queue/src/config.rs
- crates/icegate-query/src/engine/config.rs
- config/kustomize/base/values-prometheus.yaml
- crates/icegate-queue/src/reader.rs
Comment on lines
+3438
to
+3440
| "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nsum by(namespace, pod) (kube_pod_container_resource_limits{resource=\"cpu\"})", | ||
| "legendFormat": "{{namespace}}/{{pod}}", | ||
| "range": true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
if [ -f "config/kustomize/base/dashboards/Nodes-1769530183590.json" ]; then
echo "File exists"
wc -l "config/kustomize/base/dashboards/Nodes-1769530183590.json"
else
echo "File not found"
find . -name "*Nodes*.json" -o -name "*1769530183590*" 2>/dev/null | head -20
fiRepository: icegatetech/icegate
Length of output: 137
🏁 Script executed:
# Extract the relevant lines around 3438 and 3540
echo "=== Lines 3435-3445 (around 3438-3440) ==="
sed -n '3435,3445p' "config/kustomize/base/dashboards/Nodes-1769530183590.json"
echo ""
echo "=== Lines 3537-3547 (around 3540-3542) ==="
sed -n '3537,3547p' "config/kustomize/base/dashboards/Nodes-1769530183590.json"Repository: icegatetech/icegate
Length of output: 1107
🏁 Script executed:
# Search for other instances of kube_pod_container_resource_limits and kube_pod_container_resource_requests
echo "=== Instances of kube_pod_container_resource_limits ==="
rg "kube_pod_container_resource_limits" "config/kustomize/base/dashboards/Nodes-1769530183590.json" -A 2
echo ""
echo "=== Instances of kube_pod_container_resource_requests ==="
rg "kube_pod_container_resource_requests" "config/kustomize/base/dashboards/Nodes-1769530183590.json" -A 2Repository: icegatetech/icegate
Length of output: 932
🏁 Script executed:
# Check if there are other uses of clamp_min or similar guards in this file or similar dashboards
rg "clamp_min|clamp_max|bool\s+>\s+0" "config/kustomize/base/dashboards/" -A 1 | head -50Repository: icegatetech/icegate
Length of output: 3937
🌐 Web query:
PromQL clamp_min function Prometheus
💡 Result:
clamp_min(v instant-vector, min scalar) is a PromQL function that forces a lower bound on every float sample in v: any value below min becomes min, while values already ≥ min are unchanged. Histogram samples in v are silently ignored. [1]
Common use:
clamp_min(rate(http_requests_total[5m]), 0)
This is often used to prevent negative rates (e.g., from counter resets) from affecting graphs/alerts.
Related:
clamp_max(v, max)sets an upper bound. [1]clamp(v, min, max)sets both bounds (effectivelyclamp_max(clamp_min(v, min), max)). [1][2]
Sources:
[1] Prometheus “Query functions” documentation (clamp_min / clamp_max / clamp) (prometheus.io)
[2] PromLabs blog (Prometheus 2.26: clamp() as shorthand using clamp_min/clamp_max) (promlabs.com)
Citations:
- 1: https://prometheus.io/docs/prometheus/latest/querying/functions/?utm_source=openai
- 2: https://promlabs.com/blog/2021/04/01/whats-new-in-prometheus-2-26/?utm_source=openai
Guard ratio panels against zero denominators.
Lines 3438–3440 and 3540–3542 can emit +Inf/NaN when pod CPU limit or request is zero or missing, causing incorrect dashboard displays.
🔧 Proposed hardening
- "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nsum by(namespace, pod) (kube_pod_container_resource_limits{resource=\"cpu\"})",
+ "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nclamp_min(sum by(namespace, pod) (kube_pod_container_resource_limits{resource=\"cpu\"}), 1e-9)",- "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nsum by(namespace, pod) (kube_pod_container_resource_requests{resource=\"cpu\"})",
+ "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nclamp_min(sum by(namespace, pod) (kube_pod_container_resource_requests{resource=\"cpu\"}), 1e-9)",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nsum by(namespace, pod) (kube_pod_container_resource_limits{resource=\"cpu\"})", | |
| "legendFormat": "{{namespace}}/{{pod}}", | |
| "range": true, | |
| "expr": "sum by(namespace, pod) (rate(container_cpu_usage_seconds_total{pod!=\"\"}[5m]))\n/ on(namespace, pod) group_left\nclamp_min(sum by(namespace, pod) (kube_pod_container_resource_limits{resource=\"cpu\"}), 1e-9)", | |
| "legendFormat": "{{namespace}}/{{pod}}", | |
| "range": true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json` around lines 3438
- 3440, The ratio panels currently divide by sums of
kube_pod_container_resource_limits (CPU) and
kube_pod_container_resource_requests (CPU) and can produce +Inf/NaN when those
sums are zero or missing; update the PromQL expressions that compute the
denominators (the sum by(namespace, pod) of
kube_pod_container_resource_limits{resource="cpu"} and of
kube_pod_container_resource_requests{resource="cpu"}) to be guarded by clamp_min
(or wrapped with max(..., <small_nonzero>)) so the divisor is never zero (apply
the same change to both the limit and request ratio panels that use
container_cpu_usage_seconds_total divided by those sums).
…sion plans with direct Parquet metadata scans for improved performance and accuracy; introduce new error handling and scanning modules
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/icegate-query/src/engine/metadata_scan/mod.rs (1)
111-116: Consider extracting the alias mapping to avoid duplication.The
level→severity_textalias logic is also present inpredicate.rs(indexed_column_namefunction per the context snippet). Consider extracting this mapping to a shared constant or helper to ensure consistency and reduce duplication.♻️ Suggested approach
Create a shared helper in a common location:
// In a shared location like schema.rs or constants.rs pub fn resolve_label_column(label: &str) -> &str { if label == "level" { "severity_text" } else { label } }Then use it in both places.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/metadata_scan/mod.rs` around lines 111 - 116, The alias mapping for label "level" -> "severity_text" is duplicated; extract it into a shared helper (e.g., resolve_label_column) or constant and use it from both metadata_scan::mod (where label_name -> indexed_column is set) and predicate.rs (indexed_column_name) to ensure consistency; update calls in both places to invoke the new helper (referencing label_name, indexed_column_name, and the new resolve_label_column helper) and remove the inline conditional logic.crates/icegate-query/src/engine/metadata_scan/parquet_reader.rs (1)
46-91: Consider extracting common file opening logic.Both
open_file_directandopen_buildershare identical code for opening the file and creatingArrowFileReader(lines 58-63 and 82-88). Consider extracting this to a private helper to reduce duplication.♻️ Suggested refactor
async fn open_arrow_reader( file_io: &FileIO, task: &FileScanTask, ) -> Result<ArrowFileReader, MetadataScanError> { let input = file_io.new_input(&task.data_file_path).map_err(MetadataScanError::Iceberg)?; let reader = input.reader().await.map_err(MetadataScanError::Iceberg)?; let meta = FileMetadata { size: task.file_size_in_bytes, }; Ok(ArrowFileReader::new(meta, reader)) } pub async fn open_file_direct( file_io: &FileIO, task: &FileScanTask, ) -> Result<(ArrowFileReader, Arc<ParquetMetaData>), MetadataScanError> { let mut arrow_reader = open_arrow_reader(file_io, task).await?; let metadata = arrow_reader.get_metadata(None).await?; Ok((arrow_reader, metadata)) } pub async fn open_builder( file_io: &FileIO, task: &FileScanTask, ) -> Result<ParquetRecordBatchStreamBuilder<ArrowFileReader>, MetadataScanError> { let arrow_reader = open_arrow_reader(file_io, task).await?; Ok(ParquetRecordBatchStreamBuilder::new(arrow_reader).await?) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/metadata_scan/parquet_reader.rs` around lines 46 - 91, Both open_file_direct and open_builder duplicate the file-opening and ArrowFileReader construction; extract that logic into a private async helper (e.g., open_arrow_reader(file_io: &FileIO, task: &FileScanTask) -> Result<ArrowFileReader, MetadataScanError>) that performs file_io.new_input(...).map_err(MetadataScanError::Iceberg) , awaits input.reader().map_err(MetadataScanError::Iceberg), builds FileMetadata { size: task.file_size_in_bytes }, and returns ArrowFileReader::new(meta, reader). Then update open_file_direct to call the helper (make the returned arrow_reader mutable so you can call get_metadata(None).await) and update open_builder to call the helper and pass the reader into ParquetRecordBatchStreamBuilder::new(...).await?, preserving all existing error mappings and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/icegate-query/src/engine/metadata_scan/values.rs`:
- Around line 40-45: classify_label currently treats only "level" as indexed
which misroutes other indexed labels; change classify_label(name: &str) ->
LabelKind to detect indexed labels via a definitive source instead of a
hardcoded "level" check — e.g., introduce a const INDEXED_LABELS: &[&str]
(containing known indexed names like "level", "service_name", "trace_id", etc.)
or call an existing is_indexed_label(name) helper and return LabelKind::Indexed
when present, otherwise LabelKind::MapAttribute; apply the same fix to the other
identical classifier in this file so both uses use the shared lookup/const.
---
Nitpick comments:
In `@crates/icegate-query/src/engine/metadata_scan/mod.rs`:
- Around line 111-116: The alias mapping for label "level" -> "severity_text" is
duplicated; extract it into a shared helper (e.g., resolve_label_column) or
constant and use it from both metadata_scan::mod (where label_name ->
indexed_column is set) and predicate.rs (indexed_column_name) to ensure
consistency; update calls in both places to invoke the new helper (referencing
label_name, indexed_column_name, and the new resolve_label_column helper) and
remove the inline conditional logic.
In `@crates/icegate-query/src/engine/metadata_scan/parquet_reader.rs`:
- Around line 46-91: Both open_file_direct and open_builder duplicate the
file-opening and ArrowFileReader construction; extract that logic into a private
async helper (e.g., open_arrow_reader(file_io: &FileIO, task: &FileScanTask) ->
Result<ArrowFileReader, MetadataScanError>) that performs
file_io.new_input(...).map_err(MetadataScanError::Iceberg) , awaits
input.reader().map_err(MetadataScanError::Iceberg), builds FileMetadata { size:
task.file_size_in_bytes }, and returns ArrowFileReader::new(meta, reader). Then
update open_file_direct to call the helper (make the returned arrow_reader
mutable so you can call get_metadata(None).await) and update open_builder to
call the helper and pass the reader into
ParquetRecordBatchStreamBuilder::new(...).await?, preserving all existing error
mappings and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a406d1c1-104f-402b-9ec5-5a441ae730cd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/icegate-common/src/storage/cache.rscrates/icegate-query/Cargo.tomlcrates/icegate-query/src/engine/metadata_scan/error.rscrates/icegate-query/src/engine/metadata_scan/labels.rscrates/icegate-query/src/engine/metadata_scan/mod.rscrates/icegate-query/src/engine/metadata_scan/parquet_reader.rscrates/icegate-query/src/engine/metadata_scan/predicate.rscrates/icegate-query/src/engine/metadata_scan/values.rscrates/icegate-query/src/engine/mod.rscrates/icegate-query/src/logql/datafusion/planner.rscrates/icegate-query/src/loki/executor.rscrates/icegate-query/src/loki/formatters.rscrates/icegate-query/src/loki/handlers.rscrates/icegate-query/src/main.rs
💤 Files with no reviewable changes (2)
- crates/icegate-query/src/loki/formatters.rs
- crates/icegate-query/src/logql/datafusion/planner.rs
✅ Files skipped from review due to trivial changes (2)
- crates/icegate-query/Cargo.toml
- crates/icegate-common/src/storage/cache.rs
…reamline `level` alias resolution, and enhance indexed column handling for improved clarity and consistency
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/icegate-common/src/storage/cache.rs (2)
858-893:⚠️ Potential issue | 🟠 MajorPrevent stale bytes from being reinserted after a concurrent mutation.
Because Phase 2 at Line 858 runs unlocked,
fetchedcan belong to an older object version. If the same path is overwritten or deleted before Line 892, Phase 3 still merges those bytes back into the cache, so later sized reads can be served entirely from stale cache state. Re-reading the current entry is not enough here; this path needs an invalidation generation/tombstone that write/delete advance and Phase 3 checks before inserting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 858 - 893, Before releasing the lock for Phase 2, capture the current cache entry generation/tombstone (e.g., from inner.cache.get(&key) -> Option<CacheEntry> where CacheEntry carries a generation/tombstone) and pass that generation into the fetch_futures; after awaiting fetched and re-acquiring the lock (the block around inner.locks.lock(&key) and _guard), re-read the current entry generation and compare it to the captured generation and only merge/inner.cache.insert(key.clone(), merged) when they match; ensure that write/delete paths (the code that mutates inner.cache or inner.accessor) advance the generation/tombstone so this compare prevents reinserting stale bytes (update CacheValue/CacheEntry to include a generation/tombstone if not present and bump it on mutations).
752-753:⚠️ Potential issue | 🟠 MajorUse
checked_addwhen computingrange_end.Lines 752 and 790 both compute
range_endwith plainu64addition. Although file sizes in practice rarely exceed safe bounds, defensive arithmetic is necessary for any computation involving user-supplied offset and size values. The subsequent checkif range_start < range_enddoes not guard against overflow—a wrapped result could still satisfy the condition or cause subtle logic errors.Fix sketch
- let range_end = range_start + size; + let range_end = range_start + .checked_add(size) + .ok_or_else(|| Error::new(ErrorKind::Unexpected, "read range overflow"))?; ... - (range_start + size, None) + ( + range_start + .checked_add(size) + .ok_or_else(|| Error::new(ErrorKind::Unexpected, "read range overflow"))?, + None, + )Also applies to: 790–791
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 752 - 753, The computation of range_end using plain u64 addition (range_end = range_start + size) can overflow; replace both occurrences with checked_add (e.g., let range_end = range_start.checked_add(size).ok_or(/* appropriate error */)? ) and propagate or return a clear error (or handle as invalid range) instead of relying on the subsequent if range_start < range_end check; update the surrounding logic that uses range_end (the branch guarded by if range_start < range_end) to expect a validated range_end and ensure you use the same checked_add pattern for the second occurrence around the other range_end calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/icegate-query/src/engine/metadata_scan/values.rs`:
- Around line 110-122: The helper collect_map_values_from_batch currently
swallows schema mismatches; change its signature to return Result<(),
MetadataScanError> and replace the early returns with
Err(MetadataScanError::Schema(...)) when batch.schema().index_of("attributes")
fails or when the column cannot be downcast to MapArray or its keys/values
cannot be downcast to StringArray; include a clear message identifying the
batch/column and expected Map<String,String>. Update callers of
collect_map_values_from_batch to propagate or surface the error so schema
mismatches surface instead of returning partial/empty 200 OK responses.
- Around line 84-108: stream_map_values currently scans all projected batches
and ignores the row-group Predicate used by plan_files, so unrelated row groups
leak values; change the signature of stream_map_values to accept the same
Predicate (or a reference to it) passed from plan_files and apply it to the
ParquetRecordBatchStreamBuilder before building the stream (e.g. use the builder
API to set a row-group predicate or to filter row-group metadata), then build
the stream and proceed to call collect_map_values_from_batch as before; ensure
the updated function and its callers use the new parameter so /label_values only
processes row groups that satisfy the Predicate.
In `@crates/icegate-query/src/loki/executor.rs`:
- Around line 341-345: Label endpoints currently bypass WAL merges by calling
catalog().load_table() and metadata_scan::scan_labels() directly; update
execute_labels and execute_label_values to either (A) route label queries
through IcegateTableProvider::scan() so the UnionExec WAL merging path is used,
or (B) propagate WalQueryConfig into metadata_scan::scan_labels so that
scan_labels merges WAL segments into the table scan (e.g., accept a
WalQueryConfig param and apply the same merge logic used by UnionExec). Ensure
the chosen change references execute_labels, execute_label_values,
metadata_scan::scan_labels, IcegateTableProvider::scan, and
WalQueryConfig/UnionExec so label queries see fresh WAL-backed data when
wal_query_enabled is true.
---
Outside diff comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 858-893: Before releasing the lock for Phase 2, capture the
current cache entry generation/tombstone (e.g., from inner.cache.get(&key) ->
Option<CacheEntry> where CacheEntry carries a generation/tombstone) and pass
that generation into the fetch_futures; after awaiting fetched and re-acquiring
the lock (the block around inner.locks.lock(&key) and _guard), re-read the
current entry generation and compare it to the captured generation and only
merge/inner.cache.insert(key.clone(), merged) when they match; ensure that
write/delete paths (the code that mutates inner.cache or inner.accessor) advance
the generation/tombstone so this compare prevents reinserting stale bytes
(update CacheValue/CacheEntry to include a generation/tombstone if not present
and bump it on mutations).
- Around line 752-753: The computation of range_end using plain u64 addition
(range_end = range_start + size) can overflow; replace both occurrences with
checked_add (e.g., let range_end = range_start.checked_add(size).ok_or(/*
appropriate error */)? ) and propagate or return a clear error (or handle as
invalid range) instead of relying on the subsequent if range_start < range_end
check; update the surrounding logic that uses range_end (the branch guarded by
if range_start < range_end) to expect a validated range_end and ensure you use
the same checked_add pattern for the second occurrence around the other
range_end calculation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b6c4981-56b1-4a23-b7f9-caf15337d68c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/icegate-common/src/storage/cache.rscrates/icegate-query/Cargo.tomlcrates/icegate-query/src/engine/metadata_scan/error.rscrates/icegate-query/src/engine/metadata_scan/labels.rscrates/icegate-query/src/engine/metadata_scan/mod.rscrates/icegate-query/src/engine/metadata_scan/parquet_reader.rscrates/icegate-query/src/engine/metadata_scan/predicate.rscrates/icegate-query/src/engine/metadata_scan/values.rscrates/icegate-query/src/engine/mod.rscrates/icegate-query/src/logql/datafusion/planner.rscrates/icegate-query/src/loki/executor.rscrates/icegate-query/src/loki/formatters.rscrates/icegate-query/src/loki/handlers.rscrates/icegate-query/src/main.rs
💤 Files with no reviewable changes (2)
- crates/icegate-query/src/loki/formatters.rs
- crates/icegate-query/src/logql/datafusion/planner.rs
✅ Files skipped from review due to trivial changes (4)
- crates/icegate-query/Cargo.toml
- crates/icegate-query/src/engine/mod.rs
- crates/icegate-query/src/main.rs
- crates/icegate-query/src/engine/metadata_scan/parquet_reader.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/icegate-query/src/engine/metadata_scan/labels.rs
- crates/icegate-query/src/engine/metadata_scan/error.rs
…upport to label discovery, and strengthen error handling
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 813-825: The code is deep-cloning full CacheValue via
entry.value().clone() while holding the per-key lock, causing expensive memcpy
on large partially-cached objects; instead capture a cheap snapshot or change
CacheValue to hold shared-owned buffers and clone only metadata. Update the
slow-path reads in the lock section where inner.locks.lock(&key) and
inner.cache.get(&key) are used (the entry.value().clone() call and the similar
clone around lines 884-889) to avoid cloning segment payloads — either build a
lightweight Coverage/Index snapshot from the entry under the lock or modify
CacheValue to store segment buffers behind Arc/Bytes so clones are O(1), then
use that snapshot/shared-owned buffers outside the critical section for Phase 3
processing.
In `@crates/icegate-query/src/engine/metadata_scan/parquet_reader.rs`:
- Around line 229-230: The decoder helper decode_plain_byte_array_values should
fail loudly instead of silently truncating when a length prefix runs past
buf.len(): change its signature to return Result<(), Error>, validate that each
length prefix and the subsequent slice are within buf bounds and return an error
on any out-of-bounds/unterminated payload, and update callers (e.g., the
Page::DictionaryPage handling around the decode_plain_byte_array_values(&buf,
num_values as usize, out) call and the similar usage in the 317-333 range) to
propagate the Result with ? so malformed dictionary pages produce an error
rather than silent truncation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2982159-0f3c-4c78-8298-5befcb8b64c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/icegate-common/src/storage/cache.rscrates/icegate-query/Cargo.tomlcrates/icegate-query/src/engine/metadata_scan/error.rscrates/icegate-query/src/engine/metadata_scan/labels.rscrates/icegate-query/src/engine/metadata_scan/mod.rscrates/icegate-query/src/engine/metadata_scan/parquet_reader.rscrates/icegate-query/src/engine/metadata_scan/predicate.rscrates/icegate-query/src/engine/metadata_scan/values.rscrates/icegate-query/src/engine/mod.rscrates/icegate-query/src/logql/datafusion/planner.rscrates/icegate-query/src/loki/executor.rscrates/icegate-query/src/loki/formatters.rscrates/icegate-query/src/loki/handlers.rscrates/icegate-query/src/main.rs
💤 Files with no reviewable changes (2)
- crates/icegate-query/src/loki/formatters.rs
- crates/icegate-query/src/logql/datafusion/planner.rs
✅ Files skipped from review due to trivial changes (3)
- crates/icegate-query/src/engine/mod.rs
- crates/icegate-query/Cargo.toml
- crates/icegate-query/src/engine/metadata_scan/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/icegate-query/src/loki/handlers.rs
- crates/icegate-query/src/loki/executor.rs
- crates/icegate-query/src/engine/metadata_scan/error.rs
- crates/icegate-query/src/engine/metadata_scan/predicate.rs
- crates/icegate-query/src/engine/metadata_scan/labels.rs
s-prosvirnin
reviewed
Apr 9, 2026
…: delete related modules, error handling, and Parquet readers
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/icegate-common/src/storage/cache.rs (1)
892-905:⚠️ Potential issue | 🟠 MajorBound gap-fetch concurrency to prevent backend stampedes.
try_join_allspawns one concurrent backend read per gap without limits. Under highly fragmented ranges, this creates unbounded concurrent I/O and memory pressure, risking backend overload and degraded performance.Use
buffer_unorderedwith a reasonable concurrency limit (e.g., 16) to bound the number of in-flight reads:Suggested fix
-use futures::future::try_join_all; +use futures::stream::{self, StreamExt as _, TryStreamExt as _}; - let fetch_futures = gaps.iter().map(|gap| { - let gap_range = BytesRange::new(gap.start, Some(gap.end - gap.start)); - let gap_start = gap.start; - let path = path.clone(); - let inner = inner.clone(); - async move { - let (_, mut reader) = inner.accessor.read(&path, OpRead::default().with_range(gap_range)).await?; - let data = reader.read_all().await?; - Ok::<_, Error>((gap_start, data)) - } - }); - let fetched = try_join_all(fetch_futures).await?; + const MAX_GAP_FETCH_CONCURRENCY: usize = 16; + let fetched: Vec<(u64, Buffer)> = stream::iter(gaps.iter().cloned()) + .map(|gap| { + let gap_range = BytesRange::new(gap.start, Some(gap.end - gap.start)); + let gap_start = gap.start; + let path = path.clone(); + let inner = inner.clone(); + async move { + let (_, mut reader) = inner.accessor.read(&path, OpRead::default().with_range(gap_range)).await?; + let data = reader.read_all().await?; + Ok::<_, Error>((gap_start, data)) + } + }) + .buffer_unordered(MAX_GAP_FETCH_CONCURRENCY) + .try_collect() + .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 892 - 905, The current code uses try_join_all over gaps which can spawn unlimited concurrent reads and cause backend stampedes; instead, replace the unbounded try_join_all stream with a bounded concurrency pipeline by converting the gaps iterator into a stream, map each gap to the async read block that calls inner.accessor.read(...).await and reader.read_all().await, then apply buffer_unordered with a sane limit (e.g., 16) before collecting results; keep the same tuple shape (gap_start, data) and propagate errors as before so fetched retains the same Vec of (u64, bytes).
♻️ Duplicate comments (1)
crates/icegate-common/src/storage/cache.rs (1)
939-942:⚠️ Potential issue | 🟡 MinorDon’t silently drop cache-read errors in Phase 4.
Err(_) => Nonehides cache backend issues and makes incident diagnosis harder. Keep fallback behavior, but log the error path explicitly.🔎 Minimal observability fix
- let data = match inner.cache.get(&key).await { - Ok(Some(entry)) => entry.value().read_range(range_start, range_end), - _ => None, - }; + let data = match inner.cache.get(&key).await { + Ok(Some(entry)) => entry.value().read_range(range_start, range_end), + Ok(None) => None, + Err(e) => { + tracing::warn!(key = ?key, error = %e, "cache get failed during final read phase"); + None + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 939 - 942, The match on inner.cache.get(&key).await currently collapses Err(_) to None, losing error details; update the match arm for Err(e) to log the error (including e and the key or relevant identifiers) using the crate's logging facility (e.g., tracing::error! or log::error!) and then return None so behavior is unchanged but the failure is observable; target the match surrounding inner.cache.get(&key).await and the branches that call entry.value().read_range(range_start, range_end).
🧹 Nitpick comments (4)
crates/icegate-queue/src/config.rs (1)
23-29: Add focused tests for the newQueueReadConfigdefault path.
metadata_cache_capacityis now a behavior-driving setting, but the test module doesn’t currently assert its default/serde-default behavior. A small regression test set would make this safer.Proposed test additions
#[test] fn test_default_config() { let config = QueueConfig::new("s3://bucket/queue"); assert_eq!(config.common.base_path, "s3://bucket/queue"); assert_eq!(config.common.channel_capacity, 1024); assert_eq!(config.common.max_row_group_size, 8192); + assert_eq!(config.read.metadata_cache_capacity, DEFAULT_METADATA_CACHE_CAPACITY); assert_eq!(config.write.write_retries, 5); assert_eq!(config.write.records_per_flush_multiplier, 1); assert_eq!(config.write.max_bytes_per_flush, 64 * 1024 * 1024); assert_eq!(config.write.flush_interval_ms, 200); } + +#[test] +fn test_deserialize_queue_read_defaults_metadata_cache_capacity() { + let json = r#" +{ + "queue": { + "common": { + "base_path": "s3://queue" + } + } +} +"#; + let parsed = serde_json::from_str::<QueueRoot>(json).expect("valid queue config"); + assert_eq!( + parsed.queue.read.metadata_cache_capacity, + DEFAULT_METADATA_CACHE_CAPACITY + ); +} + +#[test] +fn test_deserialize_queue_read_allows_disabling_metadata_cache() { + let json = r#" +{ + "queue": { + "common": { + "base_path": "s3://queue" + }, + "read": { + "metadata_cache_capacity": 0 + } + } +} +"#; + let parsed = serde_json::from_str::<QueueRoot>(json).expect("valid queue config"); + assert_eq!(parsed.queue.read.metadata_cache_capacity, 0); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/config.rs` around lines 23 - 29, Add focused unit tests that assert the default path for QueueReadConfig produces the intended metadata_cache_capacity and that serde deserialization respects the same default when the field is omitted; specifically, add tests calling QueueReadConfig::default() and checking metadata_cache_capacity equals DEFAULT_METADATA_CACHE_CAPACITY, and a serde round-trip or deserializing from a JSON/TOML/Map without metadata_cache_capacity to ensure the deserialized QueueReadConfig (the struct used in your config module) also sets metadata_cache_capacity to DEFAULT_METADATA_CACHE_CAPACITY. Target the Default impl for QueueReadConfig and the struct used with serde to prevent regressions.crates/icegate-query/src/engine/log_metadata_scan/parquet_reader.rs (1)
374-378: VerifyLengthimplementation forOffsetChunk.The
len()method returnsself.base + self.bytes.len(), which represents the absolute file offset of the end of the chunk. This is correct forSerializedPageReaderwhich expectsLength::len()to return the total length available for reading from the "file" perspective.However, this could be confusing since typical
Lengthimplementations return the actual buffer size. A brief comment clarifying the semantics would help.📝 Suggested documentation
impl Length for OffsetChunk { + /// Returns the effective "file length" seen by `SerializedPageReader`. + /// This is `base + bytes.len()`, representing the absolute file offset + /// of the end of our in-memory slice. fn len(&self) -> u64 { self.base + self.bytes.len() as u64 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/log_metadata_scan/parquet_reader.rs` around lines 374 - 378, The Length impl for OffsetChunk (impl Length for OffsetChunk -> fn len(&self) -> u64) returns self.base + self.bytes.len() to represent the absolute file offset expected by SerializedPageReader rather than the buffer size; add a short clarifying comment on the OffsetChunk type or directly above the Length::len implementation stating that len() returns the total file-offset length (base + bytes.len()) because SerializedPageReader interprets Length as file-length, not in-memory buffer size, so future readers won't be confused.crates/icegate-query/src/engine/log_metadata_scan/values.rs (1)
132-167: Minor optimization opportunity incollect_map_values_from_batch.The
contains()check at line 160 beforeinsert()is redundant sinceBTreeSet::insertis idempotent and handles duplicates efficiently. However, the check does avoid a string allocation for duplicates, so it's a reasonable trade-off for high-cardinality MAP columns.♻️ Alternative without pre-check (simpler but allocates more)
for i in 0..keys.len() { if keys.is_valid(i) && keys.value(i) == label_name && values.is_valid(i) { let v = values.value(i); - if !out.contains(v) { - out.insert(v.to_string()); - } + out.insert(v.to_string()); } }The current approach is fine if you expect many duplicate values per batch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/log_metadata_scan/values.rs` around lines 132 - 167, The loop in collect_map_values_from_batch currently checks out.contains(v) before out.insert(v.to_string()), which is redundant because BTreeSet::insert is idempotent; simplify the code by removing the contains() branch and always calling out.insert(v.to_string()) when values.is_valid(i) and keys.value(i) == label_name, keeping the rest of the function unchanged (function: collect_map_values_from_batch, variables: keys, values, out, methods: contains(), insert()); note this will allocate for duplicates but makes the code simpler.crates/icegate-query/src/engine/log_metadata_scan/predicate.rs (1)
63-82: Alias mappings are already consistent; consider extracting to a shared location to avoid future divergence.Both
indexed_column_name(predicate.rs) andmap_label_to_internal_name(planner.rs) handle identical aliases:
levelanddetected_level→severity_textservice→service_nameNo inconsistency exists. Extracting this mapping to a shared constant or utility function is still a reasonable optional refactoring to reduce duplication and ensure both locations remain aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/log_metadata_scan/predicate.rs` around lines 63 - 82, indexed_column_name in predicate.rs duplicates the alias mapping logic found in LogQLPlanner::map_label_to_internal_name in planner.rs, so extract the shared alias-to-internal mapping into a single reusable place (e.g., a const map or small utility function) and update both indexed_column_name and LogQLPlanner::map_label_to_internal_name to delegate to that shared symbol; ensure the shared symbol covers LEVEL_ALIAS, "detected_level" -> "severity_text" and "service" -> "service_name", adjust imports/visibility as needed, and run/update any tests that reference the previous functions to use the consolidated mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 892-905: The current code uses try_join_all over gaps which can
spawn unlimited concurrent reads and cause backend stampedes; instead, replace
the unbounded try_join_all stream with a bounded concurrency pipeline by
converting the gaps iterator into a stream, map each gap to the async read block
that calls inner.accessor.read(...).await and reader.read_all().await, then
apply buffer_unordered with a sane limit (e.g., 16) before collecting results;
keep the same tuple shape (gap_start, data) and propagate errors as before so
fetched retains the same Vec of (u64, bytes).
---
Duplicate comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 939-942: The match on inner.cache.get(&key).await currently
collapses Err(_) to None, losing error details; update the match arm for Err(e)
to log the error (including e and the key or relevant identifiers) using the
crate's logging facility (e.g., tracing::error! or log::error!) and then return
None so behavior is unchanged but the failure is observable; target the match
surrounding inner.cache.get(&key).await and the branches that call
entry.value().read_range(range_start, range_end).
---
Nitpick comments:
In `@crates/icegate-query/src/engine/log_metadata_scan/parquet_reader.rs`:
- Around line 374-378: The Length impl for OffsetChunk (impl Length for
OffsetChunk -> fn len(&self) -> u64) returns self.base + self.bytes.len() to
represent the absolute file offset expected by SerializedPageReader rather than
the buffer size; add a short clarifying comment on the OffsetChunk type or
directly above the Length::len implementation stating that len() returns the
total file-offset length (base + bytes.len()) because SerializedPageReader
interprets Length as file-length, not in-memory buffer size, so future readers
won't be confused.
In `@crates/icegate-query/src/engine/log_metadata_scan/predicate.rs`:
- Around line 63-82: indexed_column_name in predicate.rs duplicates the alias
mapping logic found in LogQLPlanner::map_label_to_internal_name in planner.rs,
so extract the shared alias-to-internal mapping into a single reusable place
(e.g., a const map or small utility function) and update both
indexed_column_name and LogQLPlanner::map_label_to_internal_name to delegate to
that shared symbol; ensure the shared symbol covers LEVEL_ALIAS,
"detected_level" -> "severity_text" and "service" -> "service_name", adjust
imports/visibility as needed, and run/update any tests that reference the
previous functions to use the consolidated mapping.
In `@crates/icegate-query/src/engine/log_metadata_scan/values.rs`:
- Around line 132-167: The loop in collect_map_values_from_batch currently
checks out.contains(v) before out.insert(v.to_string()), which is redundant
because BTreeSet::insert is idempotent; simplify the code by removing the
contains() branch and always calling out.insert(v.to_string()) when
values.is_valid(i) and keys.value(i) == label_name, keeping the rest of the
function unchanged (function: collect_map_values_from_batch, variables: keys,
values, out, methods: contains(), insert()); note this will allocate for
duplicates but makes the code simpler.
In `@crates/icegate-queue/src/config.rs`:
- Around line 23-29: Add focused unit tests that assert the default path for
QueueReadConfig produces the intended metadata_cache_capacity and that serde
deserialization respects the same default when the field is omitted;
specifically, add tests calling QueueReadConfig::default() and checking
metadata_cache_capacity equals DEFAULT_METADATA_CACHE_CAPACITY, and a serde
round-trip or deserializing from a JSON/TOML/Map without metadata_cache_capacity
to ensure the deserialized QueueReadConfig (the struct used in your config
module) also sets metadata_cache_capacity to DEFAULT_METADATA_CACHE_CAPACITY.
Target the Default impl for QueueReadConfig and the struct used with serde to
prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34199b5a-6172-4b12-9675-3ea0cfa58fcb
📒 Files selected for processing (13)
crates/icegate-common/src/schema.rscrates/icegate-common/src/storage/cache.rscrates/icegate-query/src/engine/log_metadata_scan/error.rscrates/icegate-query/src/engine/log_metadata_scan/labels.rscrates/icegate-query/src/engine/log_metadata_scan/mod.rscrates/icegate-query/src/engine/log_metadata_scan/parquet_reader.rscrates/icegate-query/src/engine/log_metadata_scan/predicate.rscrates/icegate-query/src/engine/log_metadata_scan/values.rscrates/icegate-query/src/engine/mod.rscrates/icegate-query/src/logql/datafusion/planner.rscrates/icegate-query/src/loki/executor.rscrates/icegate-query/src/loki/formatters.rscrates/icegate-queue/src/config.rs
✅ Files skipped from review due to trivial changes (1)
- crates/icegate-query/src/engine/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/icegate-query/src/loki/executor.rs
- crates/icegate-query/src/loki/formatters.rs
…h `metadata_entries_cache_capacity` for improved clarity across WAL reader, config, and Helm values
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/icegate-common/src/storage/cache.rs (1)
892-905:⚠️ Potential issue | 🟠 MajorCap the gap-fetch fan-out.
try_join_allfires one backend read per gap. On a fragmented entry, a single logical read can explode into many concurrent object-store calls, which is an avoidable latency/throttling risk on the slow path. Please bound this withbuffer_unordered/a semaphore, or coalesce nearby gaps before dispatching reads.Possible direction
- let fetch_futures = gaps.iter().map(|gap| { - let gap_range = BytesRange::new(gap.start, Some(gap.end - gap.start)); - let gap_start = gap.start; - let path = path.clone(); - let inner = inner.clone(); - async move { - let (_, mut reader) = inner.accessor.read(&path, OpRead::default().with_range(gap_range)).await?; - let data = reader.read_all().await?; - Ok::<_, Error>((gap_start, data)) - } - }); - let fetched = try_join_all(fetch_futures).await?; + let fetched = futures::stream::iter(gaps.iter().cloned()) + .map(|gap| { + let path = path.clone(); + let inner = inner.clone(); + async move { + let gap_range = BytesRange::new(gap.start, Some(gap.end - gap.start)); + let (_, mut reader) = inner.accessor.read(&path, OpRead::default().with_range(gap_range)).await?; + let data = reader.read_all().await?; + Ok::<_, Error>((gap.start, data)) + } + }) + .buffer_unordered(8) + .try_collect::<Vec<_>>() + .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 892 - 905, The current use of try_join_all over gaps (building per-gap BytesRange::new + inner.accessor.read with OpRead and producing fetched) can spawn unbounded concurrent reads; limit the fan-out by replacing try_join_all with a bounded executor pattern: either convert the gaps.iterator into a stream and use buffer_unordered(N) to cap concurrency, or acquire a tokio::sync::Semaphore before each async read to enforce a max outstanding reads value, then await all results into fetched; alternatively coalesce adjacent gaps into larger ranges before dispatching reads to reduce the number of tasks. Ensure the change targets the block that constructs fetch_futures/gaps and the read call via inner.accessor.read so behavior and error propagation remain the same.
♻️ Duplicate comments (2)
crates/icegate-common/src/storage/cache.rs (2)
1445-1478:⚠️ Potential issue | 🟡 MinorThese tests never exercise the miss/lock path they describe.
Both tests seed data via
op.write(...), butCacheWriter::close()eagerly inserts the full object into the cache on Line 1083. That makes the subsequent reads warm hits, so these cases do not cover the new miss/fetch/merge logic or the per-key locking behavior. Seed the backend with an uncached operator, or bypass write-time cache population for these tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 1445 - 1478, The tests test_read_cache_miss_populates_cache and test_concurrent_reads_same_key never exercise the cache-miss path because op.write(...) and CacheWriter::close() eagerly populate the cache; change the test setup so the backend object is written without populating the cache (for example, write the object using an operator created with caching disabled or by using a non-cached operator instance returned from build_test_operator for the initial write) and then perform reads with the cached operator, or stub/bypass CacheWriter::close() cache insertion for the seeded write; ensure you reference the op.write(...) call for seeding and then call op.read(...) (or cloned ops) to validate the miss/fetch/merge and per-key locking behavior.
836-928:⚠️ Potential issue | 🔴 CriticalThe new read protocol still races with write/delete invalidation.
These phases serialize readers with
inner.locks, but Lines 1080-1084 and 1136-1141 mutate/remove the same key without taking that lock. A write or delete between the gap fetch and merge can repopulate the cache with stale bytes, and the Phase 4 fallback will also serve them after invalidation. Please coordinate write/delete with the same per-key lock, or reject the merge/fallback unless the key still matches the Phase 1 state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/cache.rs` around lines 836 - 928, Readers' three-phase read (gaps/snapshot/fetch/merge) races with concurrent writers/deletes because code mutating the same key (inner.cache.insert / removals) at lines that don't take inner.locks can repopulate or remove the key between the fetch and merge; fix by coordinating with the same per-key lock or by validating the cache state before merging: either (A) ensure all writers/deleters acquire inner.locks.lock(&key) (same lock used in Phase 1/3) before calling inner.cache.insert/remove, or (B) in the Phase 3 merge (the block that re-reads inner.cache.get and builds merged from existing.unwrap_or_else(CacheValue::new) and then inner.cache.insert), compare the fresh existing entry to the Phase 1 snapshot (the variable snapshot) and abort/skipping the merge (and ensure Phase 4 fallback only serves snapshot ∪ fetched when snapshot still matches current cache) if they differ; implement one of these approaches and apply the same check to the Phase 4 fallback path so stale bytes are never merged or served.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 258-280: The custom Serialize/Deserialize impls for CacheValue
currently serialize only the inner BTreeMap<u64, Vec<u8>> which is not
compatible with the previous derived struct representation (which included
struct metadata and the field name), so the comment claiming on-wire
compatibility is incorrect; either revert to a derived impl (e.g., restore
#[derive(Serialize, Deserialize)] on CacheValue by providing serde support for
Bytes or a wrapper type), or implement a safe migration in the Deserialize for
CacheValue: attempt to deserialize the incoming bytes into the old wrapped
struct shape (a small helper struct with a segments: BTreeMap<u64, Vec<u8>>
field) and fall back to the new BTreeMap<u64, Vec<u8>> format if that fails,
then convert Vec<u8> into Bytes; alternatively, simply update the comment to
acknowledge this breaking change if migration/backward compatibility is not
required.
---
Outside diff comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 892-905: The current use of try_join_all over gaps (building
per-gap BytesRange::new + inner.accessor.read with OpRead and producing fetched)
can spawn unbounded concurrent reads; limit the fan-out by replacing
try_join_all with a bounded executor pattern: either convert the gaps.iterator
into a stream and use buffer_unordered(N) to cap concurrency, or acquire a
tokio::sync::Semaphore before each async read to enforce a max outstanding reads
value, then await all results into fetched; alternatively coalesce adjacent gaps
into larger ranges before dispatching reads to reduce the number of tasks.
Ensure the change targets the block that constructs fetch_futures/gaps and the
read call via inner.accessor.read so behavior and error propagation remain the
same.
---
Duplicate comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 1445-1478: The tests test_read_cache_miss_populates_cache and
test_concurrent_reads_same_key never exercise the cache-miss path because
op.write(...) and CacheWriter::close() eagerly populate the cache; change the
test setup so the backend object is written without populating the cache (for
example, write the object using an operator created with caching disabled or by
using a non-cached operator instance returned from build_test_operator for the
initial write) and then perform reads with the cached operator, or stub/bypass
CacheWriter::close() cache insertion for the seeded write; ensure you reference
the op.write(...) call for seeding and then call op.read(...) (or cloned ops) to
validate the miss/fetch/merge and per-key locking behavior.
- Around line 836-928: Readers' three-phase read (gaps/snapshot/fetch/merge)
races with concurrent writers/deletes because code mutating the same key
(inner.cache.insert / removals) at lines that don't take inner.locks can
repopulate or remove the key between the fetch and merge; fix by coordinating
with the same per-key lock or by validating the cache state before merging:
either (A) ensure all writers/deleters acquire inner.locks.lock(&key) (same lock
used in Phase 1/3) before calling inner.cache.insert/remove, or (B) in the Phase
3 merge (the block that re-reads inner.cache.get and builds merged from
existing.unwrap_or_else(CacheValue::new) and then inner.cache.insert), compare
the fresh existing entry to the Phase 1 snapshot (the variable snapshot) and
abort/skipping the merge (and ensure Phase 4 fallback only serves snapshot ∪
fetched when snapshot still matches current cache) if they differ; implement one
of these approaches and apply the same check to the Phase 4 fallback path so
stale bytes are never merged or served.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43d9d369-9e5f-411d-a811-d3e7f460f1df
📒 Files selected for processing (8)
config/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlcrates/icegate-common/src/storage/cache.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/engine/config.rscrates/icegate-query/src/engine/log_metadata_scan/parquet_reader.rscrates/icegate-queue/src/config.rscrates/icegate-queue/src/reader.rs
✅ Files skipped from review due to trivial changes (1)
- crates/icegate-query/src/engine/log_metadata_scan/parquet_reader.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- config/helm/icegate/templates/configmap-query.yaml
- config/helm/icegate/values.yaml
- crates/icegate-queue/src/config.rs
- crates/icegate-query/src/engine/config.rs
- crates/icegate-queue/src/reader.rs
s-prosvirnin
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Performance Improvements
Configuration