GH-91: Refactor IO cache to support prefetch#92
Merged
frisbeeman merged 19 commits intomainfrom Mar 24, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughRenames IoCacheHandle→IoHandle; introduces a foyer-backed byte-range StorageCache, Parquet prefetch layer, and IceGateStorage/StorageFactory; wires cache/prefetch/stat TTL and max-write-cache-size through object-store creation; adds background catalog-provider refresh; removes write-grouping from queue writer; updates workspace deps, Helm/Kustomize values, metrics, and dashboards. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Op as OpenDAL Operator
participant Prefetch as PrefetchLayer
participant Cache as StorageCache
participant S3 as S3 Backend
Client->>Op: read(path, offset, len)
Op->>Prefetch: prefetch-aware read(path, offset, len)
Prefetch->>Prefetch: check footer? / plan prefetch
Prefetch->>Cache: check coverage for [offset,offset+len)
Cache->>Cache: fast-path coverage check
alt covered
Cache-->>Op: return cached bytes
else not covered
Cache->>Cache: acquire per-key lock, compute gaps
Cache->>S3: fetch missing ranges (concurrent)
S3-->>Cache: returned bytes
Cache->>Cache: merge segments, release lock
Cache-->>Op: return requested subrange
end
Op-->>Client: data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 13
🧹 Nitpick comments (16)
crates/icegate-query/src/engine/provider/scan.rs (1)
196-196: Avoid a hardcoded batch-size literal at Line 196.Consider replacing
8192with a named constant (or config-fed value) to keep tuning centralized.♻️ Suggested cleanup
+const DEFAULT_SCAN_BATCH_SIZE: usize = 8_192; ... - let table_scan = scan_builder.with_batch_size(Some(8192)).build().map_err(to_datafusion_error)?; + let table_scan = scan_builder + .with_batch_size(Some(DEFAULT_SCAN_BATCH_SIZE)) + .build() + .map_err(to_datafusion_error)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/provider/scan.rs` at line 196, Replace the hardcoded batch size literal used in scan_builder.with_batch_size(Some(8192)) by introducing a named constant or configuration value (e.g., BATCH_SIZE or a runtime-config field) and use that symbol instead; update the call in the scan construction (the scan_builder.with_batch_size(...) invocation that produces table_scan) to pass the new constant or config value so batch-size tuning is centralized and not hardcoded.crates/icegate-query/src/engine/provider/table.rs (1)
246-246: Add a regression test for invalid projection indices.Now that projection setup can return an error, please add a focused test that an out-of-range projection index fails planning deterministically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/provider/table.rs` at line 246, Add a focused regression test that supplies an out-of-range projection index to the projection setup and asserts planning fails deterministically: create a unit/integration test in the icegate-query crate that builds the same planner/table builder used in table.rs, call with_projection_indices(Some(proj.clone())) (or invoke the public planning entry-point that triggers that call) with a projection Vec containing an index >= number of table columns, and assert the result is an Err (use an exact error variant or message check to ensure determinism). Reference the builder.with_projection_indices call in table.rs and the planner/table provider setup so the test exercises the same code path and fails reliably on invalid indices.config/helm/icegate/templates/deployment-query.yaml (1)
98-104: Add a guard for persistence + multi-replica query deployments.This mounts one named PVC from a Deployment. If replicas are scaled and the PVC is
ReadWriteOnce, query pods can become unschedulable.Suggested fail-fast guard
{{- if .Values.query.cache.enabled }} + {{- if and .Values.query.cache.persistence.enabled (gt (int .Values.query.replicaCount) 1) }} + {{- fail "query.cache.persistence.enabled with Deployment replicas > 1 requires RWX-capable storage" }} + {{- end }} - name: cache {{- if .Values.query.cache.persistence.enabled }} persistentVolumeClaim: claimName: {{ include "icegate.cachePvcName" . }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/helm/icegate/templates/deployment-query.yaml` around lines 98 - 104, When persistence is enabled for query cache ({{ .Values.query.cache.persistence.enabled }}) and the Deployment is scaled to multiple replicas (e.g. {{ .Values.query.replicaCount }} > 1), a single ReadWriteOnce PVC ({{ include "icegate.cachePvcName" . }}) will make pods unschedulable; add a fail-fast guard in the deployment template: check if .Values.query.cache.persistence.enabled is true AND .Values.query.replicaCount (or .Values.query.replicaCount | int) > 1, then call Helm's fail function with a clear message (e.g. "persistence.enabled cannot be true when query.replicaCount > 1 with RWO PVC") so the chart install/upgrade aborts instead of producing an unschedulable Deployment.config/kustomize/overlays/aws-glue/values-icegate.yaml (1)
46-54: Consider addingfsTypeto the storageClass configuration.The storageClass configuration specifies
type,iops,throughput, andencrypted, but omitsfsType. While it will default toext4from the basevalues.yaml, explicitly specifying it here ensures the overlay is self-documenting for production deployments.📝 Suggested addition
storageClass: create: true type: gp3 iops: "6000" throughput: "500" encrypted: "true" + fsType: ext4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/aws-glue/values-icegate.yaml` around lines 46 - 54, The persistence.storageClass block in values-icegate.yaml omits fsType; add an explicit fsType (e.g., fsType: "ext4") under the storageClass for persistence to make the overlay self-documenting and to override defaults from base values.yaml; update the persistence.storageClass configuration (type, iops, throughput, encrypted) to include fsType so production manifests explicitly declare the filesystem type.crates/icegate-common/Cargo.toml (1)
46-49: Nit:futuresis misplaced under the Logging section.Consider moving
futuresto a more appropriate section (e.g., neartokioandtokio-utilat lines 51-52, or create an "# Async utilities" section) for better organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/Cargo.toml` around lines 46 - 49, Move the misplaced dependency entry for "futures" out of the "# Logging" block in Cargo.toml and place it near the other async-related dependencies (e.g., next to "tokio" and "tokio-util") or under a new "# Async utilities" section; specifically update the "futures = { workspace = true }" line so it's grouped with async/runtime deps for clearer organization while leaving the tracing and tracing-subscriber entries in the Logging section unchanged.crates/icegate-query/src/engine/provider/schema.rs (1)
55-75: Consider bounding table-load concurrency for large namespaces.This approach loads all tables concurrently via
try_join_all, which is intentional for latency (as noted in the code comment). However, on namespaces with many tables, this creates an unbounded fan-out of concurrent catalog REST calls that could hit throttling or timeouts on some catalog implementations. If typical deployments have large namespaces, consider capping parallelism withbuffer_unorderedor a semaphore to preserve the concurrency benefit while avoiding overload bursts. Note that the same pattern is used incatalog.rsas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/provider/schema.rs` around lines 55 - 75, The current tables_loaded creation uses try_join_all over table_idents which can spawn unbounded concurrent catalog calls; change this to a bounded concurrency approach (e.g., convert table_idents.iter() into a futures::stream::iter and use .map(async move { ... }) then .buffer_unordered(CONCURRENCY).collect::<Result<Vec<_>, _>>().await or protect the body with a tokio::sync::Semaphore) so that calls that invoke IcegateTableProvider::try_new, IcebergStaticTableProvider::try_new_from_table, and catalog.load_table are limited; introduce a configurable CONCURRENCY constant or config value and replace try_join_all usage with the bounded stream/semaphore pattern while preserving the same per-item logic (namespace, wal_reader, name, provider) and result shape.crates/icegate-common/src/storage/icegate_s3.rs (1)
67-69: Avoid double-normalizing booleans ins3_config_parse.
is_truthyalready lowercases input, but callers passto_lowercase()first (e.g., Line 68, Line 106, Line 111, Line 116). You can pass the original string directly to reduce temporary allocations.♻️ Suggested cleanup
- cfg.enable_virtual_host_style = !is_truthy(path_style_access.to_lowercase().as_str()); + cfg.enable_virtual_host_style = !is_truthy(&path_style_access); - && is_truthy(allow_anonymous.to_lowercase().as_str()) + && is_truthy(&allow_anonymous) - && is_truthy(disable_ec2_metadata.to_lowercase().as_str()) + && is_truthy(&disable_ec2_metadata) - && is_truthy(disable_config_load.to_lowercase().as_str()) + && is_truthy(&disable_config_load)Also applies to: 105-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_s3.rs` around lines 67 - 69, In s3_config_parse, avoid double-normalizing boolean strings: when removing entries like S3_PATH_STYLE_ACCESS (and other keys in the same block around lines handling path style, assume role, use_tls, etc.), pass the original string value directly into is_truthy rather than calling to_lowercase() first; update the call site that currently does path_style_access.to_lowercase().as_str() to just pass path_style_access.as_str() (and do the same for the other occurrences) so you remove the extra temporary allocation while keeping the boolean normalization inside is_truthy; ensure cfg.enable_virtual_host_style and related assignments still invert/assign the result as before.crates/icegate-query/src/engine/core.rs (2)
236-260: Cold/stale path can trigger concurrent synchronous rebuilds.When cache is stale under high concurrency, multiple requests can call
build_providersimultaneously. Consider a singleflight/mutex gate around rebuild to avoid thundering-herd catalog calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/core.rs` around lines 236 - 260, get_provider can trigger concurrent synchronous calls to build_provider when the cache is stale; add an async singleflight gate so only one rebuild runs and others await its result. Introduce a rebuild guard on the engine (e.g., a tokio::sync::Mutex or a singleflight-like primitive stored on the struct, name it provider_rebuild_lock or provider_rebuild_inflight) and wrap the cold/stale path: after checking provider_rx and determining rebuild is needed, acquire the guard, re-check provider_rx (to avoid TOCTOU), run build_provider once if still needed, send the CachedProvider via provider_tx, then release the guard so concurrent callers receive the rebuilt provider instead of launching duplicate builds.
133-178: Add a one-shot guard to enforce single invocation ofstart_background_refresh.The function is documented as "must be called once after construction" but has no mechanism to prevent multiple calls. If called repeatedly, it spawns multiple refresh tasks, duplicating catalog rebuild load. Consider using
std::sync::Onceortokio::sync::Mutex<bool>to enforce single execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/core.rs` around lines 133 - 178, start_background_refresh currently spawns a new tokio task every time it is called, which can duplicate work; add a one-shot guard (e.g., a std::sync::Once or a std::sync::atomic::AtomicBool/AtomicU8 field like background_refresh_started on the Engine struct) and update start_background_refresh to check-and-set that guard at the start and return immediately if already invoked; ensure the guard is initialized with the Engine and is accessed atomically/thread-safely before cloning Arc<Self> and spawning the task so only the first caller spawns the refresh loop.crates/icegate-common/src/storage/cache.rs (6)
716-718: Unnecessary double allocation when merging fetched data.
data.to_vec()allocates aVec<u8>, then&data.to_vec()borrows it as a slice. Sinceinsert_rangetakes&[u8], you can avoid the intermediate allocation.Avoid intermediate Vec allocation
for (offset, data) in &fetched { - merged.insert_range(*offset, &data.to_vec()); + merged.insert_range(*offset, data.chunk()); }If
Buffer::chunk()isn't available,.as_ref()or iterating over chunks may work depending on the OpenDAL version.🤖 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 716 - 718, The loop unnecessarily allocates a Vec via data.to_vec() before borrowing it; change the call to pass a slice directly to merged.insert_range by converting the fetched buffer to a byte slice (e.g., use data.as_ref() or Buffer::chunk()/as_slice depending on OpenDAL version) so that insert_range(offset, &[u8]) receives a borrowed slice instead of &data.to_vec(); update the loop that iterates over fetched and use the direct slice conversion for each data item passed to merged.insert_range.
1014-1020: Test cache uses memory-only mode without explicit disk device.The test helper builds a cache with
.storage().build()which may use a default in-memory storage engine. This is appropriate for unit tests but differs from production's disk-backed configuration. Consider adding an integration test that exercises the disk path if not covered elsewhere.🤖 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 1014 - 1020, The test helper constructs a StorageCache via foyer::HybridCacheBuilder::new().memory(...).with_weighter(...).storage().build() which only exercises an in-memory storage path; update the test(s) to also exercise the disk-backed path by adding a disk device to the builder (e.g., use the HybridCacheBuilder storage API that accepts a disk device or storage_with_device before build) or add a separate integration test that constructs StorageCache with a real temporary disk device and verifies behavior; target the code around StorageCache / HybridCacheBuilder / .storage().build() and the weighter using CacheKey and CacheValue to implement this change.
815-818: Writer buffers entire file in memory before caching.For large files,
QueueBufaccumulates all written data before populating the cache on close. This could cause memory pressure for multi-gigabyte writes. Consider streaming directly to cache or limiting cache population to files below a size threshold.🤖 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 815 - 818, QueueBuf's write method currently pushes every Buffer into self.buf causing the entire file to be buffered in memory; change it so writes stream directly to the underlying writer (self.w.write) and only buffer up to a configurable threshold: track total_buffered_size in QueueBuf, when total exceeds MAX_BUFFER_SIZE stop appending to self.buf and rely on streaming to self.w, and on close only populate the cache if total_buffered_size <= MAX_BUFFER_SIZE (or implement incremental cache population during writes). Update QueueBuf::write, QueueBuf fields (add total_buffered_size and max_buffer_size), and QueueBuf::close logic accordingly to avoid buffering multi-gigabyte files.
255-262: 32-bit truncation is documented but could silently corrupt data.The
u64_to_usizehelper saturates on 32-bit targets, which could cause silent data corruption if somehow used with >4GiB offsets. Consider returningOption<usize>or panicking with a clear message instead of silent saturation.Consider explicit error handling
-#[allow(clippy::cast_possible_truncation)] -const fn u64_to_usize(v: u64) -> usize { - v as usize +const fn u64_to_usize(v: u64) -> usize { + #[cfg(target_pointer_width = "64")] + { + v as usize + } + #[cfg(not(target_pointer_width = "64"))] + { + assert!(v <= usize::MAX as u64, "offset exceeds platform address space"); + v as usize + } }🤖 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 255 - 262, The helper u64_to_usize silently truncates on 32-bit targets which can corrupt offsets; change its signature from const fn u64_to_usize(v: u64) -> usize to return Option<usize> (e.g., const fn u64_to_usize(v: u64) -> Option<usize>) and return Some(v as usize) only when v <= usize::MAX, otherwise None; remove the clippy allow attribute and update all call sites (functions/methods that call u64_to_usize) to propagate or handle the None case (return an error or panic with a clear message) so overflow is detected explicitly instead of being silently truncated.
749-758: Expired stat cache entries are only evicted on access.Stale entries remain in memory until the same key is accessed again. For workloads with many unique paths that are never re-accessed, this causes unbounded growth. Consider adding a background reaper task or using a TTL-aware cache like
moka.🤖 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 749 - 758, The current stat cache (inner.stat_cache) only evicts expired entries on access (the inserted_at/ttl check in the get path), causing unbounded growth for one-shot keys; add either a background reaper task that periodically scans inner.stat_cache and removes entries whose inserted_at.elapsed() >= ttl (e.g., spawn a cleanup task at cache initialization that iterates and calls inner.stat_cache.remove(key) for expired entries), or replace inner.stat_cache with a TTL-aware cache like moka and use its time-based eviction APIs; ensure the cleanup uses the same inserted_at/ttl semantics so RpStat construction remains unchanged.
441-464: Per-key locks accumulate without bound.
KeyLocksentries are only removed on explicit delete (line 883), not when foyer evicts cache entries. For long-running processes with many unique paths, theDashMapwill grow monotonically.Consider periodic cleanup or weak references if this becomes a memory concern in production.
🤖 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 441 - 464, KeyLocks currently stores strong Arcs in a DashMap and never evicts entries except via explicit remove, causing unbounded growth; change the map to hold Weak<tokio::sync::Mutex<()>> and update KeyLocks::lock to attempt upgrading the Weak::upgrade(), and if upgrade fails insert a new Arc and store its Weak, returning the Arc; also add a small cleanup helper (e.g., KeyLocks::prune_dead or perform occasional trimming inside lock) that removes entries whose Weak::upgrade() returns None so stale keys don't accumulate; keep KeyLocks::remove for explicit deletion but prefer the Weak-based approach to avoid monotonic memory growth (refer to types KeyLocks, lock, remove, CacheKey, and DashMap in the diff).crates/icegate-common/src/storage/icegate_storage.rs (1)
298-307: Factory unconditionally builds S3 variant.
StorageFactory::buildalways createsIceGateStorage::S3, even ifStorageConfigspecifies a different scheme. If the config contains memory or unsupported schemes,s3_config_parsewill either fail confusingly or produce invalid configuration.Consider checking the scheme from config properties and returning an appropriate variant or error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 298 - 307, The build method currently always constructs IceGateStorage::S3 using s3_config_parse, which ignores StorageConfig.scheme and can fail or produce invalid configs; update the StorageFactory::build (fn build(&self, config: &StorageConfig) -> Result<Arc<dyn Storage>>) to inspect the scheme from config.props() or config.scheme() and branch: for "s3" parse with s3_config_parse and return IceGateStorage::S3, for "memory" return the appropriate IceGateStorage::Memory variant (or other supported variants) using their respective parsers/constructors, and for unknown schemes return a clear error result instead of blindly calling s3_config_parse. Ensure to reference IceGateStorage::S3, s3_config_parse, StorageConfig, and build in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 75-86: Update the top comment in Cargo.toml that currently reads
"DataFusion 51 + Arrow 57 compat" to reflect DataFusion 52 compatibility (e.g.,
"DataFusion 52 + Arrow 57 compat") so it matches the actual dependency
declaration datafusion = "52.2" and the pinned iceberg-rust revision entries
(iceberg, iceberg-catalog-rest, iceberg-catalog-s3tables, iceberg-catalog-glue,
iceberg-datafusion).
- Around line 56-57: Workspace pins opendal to a specific git revision while
iceberg pulls a different opendal from crates.io, causing duplicate opendal
versions; update the dependency alignment by either (A) bumping the iceberg git
rev to one that uses the same opendal git rev as our workspace or (B) overriding
iceberg's opendal to the workspace git rev via a Cargo.toml [patch.crates-io] or
[replace] entry; target the opendal and object_store_opendal entries in
Cargo.toml and verify the iceberg dependency revision (the rev referenced for
iceberg) is updated or patched so all crates resolve to the same opendal source.
In `@config/helm/icegate/templates/configmap-query.yaml`:
- Around line 18-20: The Helm template currently uses a "with
.Values.query.cache.statTtlSecs" block which will skip rendering when
statTtlSecs is explicitly set to 0; replace the "with" check with an explicit
key-existence check using if hasKey .Values.query.cache "statTtlSecs" and then
render stat_ttl_secs using the explicit value (e.g., reference
.Values.query.cache.statTtlSecs) so that zero is preserved; update the template
around the stat_ttl_secs block and remove the "with
.Values.query.cache.statTtlSecs" usage.
In `@crates/icegate-common/src/catalog/builder.rs`:
- Around line 70-83: from_config currently preserves prefetch_config even when
build_storage_cache() returns None, which lets prefetch propagate with no cache;
update IoHandle::from_config (the from_config async fn that calls
build_storage_cache) to either return an Err when prefetch_config is Some and no
cache was built, or to clear prefetch (set prefetch to None) when cache is None;
locate the match on cache_config and the Ok(Self { cache, prefetch:
prefetch_config, ... }) and change the logic so prefetch is only kept if
cache.is_some() (or return a clear error referencing prefetch+no-cache) so
storage_factory()/catalog IO never receives a prefetch without an underlying
cache.
In `@crates/icegate-common/src/storage/cache.rs`:
- Line 18: Remove the unused import symbol DeviceBuilder from the foyer import
list; the module only uses HybridCache and FsDeviceBuilder (see HybridCache
usage around the cache implementation and FsDeviceBuilder usage near the device
builder instantiation), so update the use statement to import only HybridCache
(and any other actually used symbols) and eliminate DeviceBuilder to fix the
unused-import warning.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 7-16: Update the module-level docstring to match the actual
wrapper construction order: replace the current "OtelMetrics → FoyerCache →
OtelTrace → Retry → S3" stack with "Prefetch → FoyerCache → OtelMetrics →
OtelTrace → Retry → S3" and adjust bullet descriptions so they reflect the real
behavior (add a Prefetch bullet noting it triggers background reads for Parquet
metadata; change OtelMetrics bullet to indicate it records metrics for cache
misses / S3 round-trips while FoyerCache short-circuits reads and OtelTrace
traces actual S3 round-trips and Retry handles transient failures). Ensure the
docstring text surrounds the triple-backtick block and bullets near the top of
the file are edited to reference the Prefetch, FoyerCache, OtelMetrics,
OtelTrace, Retry, and S3 layers consistently with the code that constructs
Prefetch → Cache → OtelMetrics → OtelTrace → Retry → S3.
- Around line 86-89: The Memory match arm's path stripping is fragile: replace
the current unwrap_or_else fallback in the Self::Memory(op) => { ... } block so
we don't blindly drop the first character; instead detect valid memory prefixes
and produce a clear relative path or return an error. Concretely, check for
"memory:/" and "memory:" prefixes on the incoming path (using strip_prefix twice
or starts_with + strip_prefix), compute relative by removing the prefix and
trimming any leading '/' (e.g., trim_start_matches('/')), and if neither prefix
is present return an Err (matching the S3 validation style) rather than using
path.get(1..).unwrap_or(path). Ensure you update the Ok((op.clone(), relative))
return to use the new validated relative path.
- Around line 112-128: The OtelMetrics layer is currently applied outside the
cache, contradicting the comment; change the layering so metrics sit below the
cache by applying CacheLayer before OtelMetricsLayer. Concretely, when building
`operator` with `bare.layer(...)`, ensure you call
`.layer(CacheLayer::new(...))` (inside the `if let Some(fc) = cache` block)
before applying `.layer(OtelMetricsLayer::builder().register(m))` (inside the
`if let Some(m) = meter` block), so `OtelMetricsLayer` is wrapped underneath
`CacheLayer` and only observes real S3 round-trips.
In `@crates/icegate-common/src/storage/prefetch.rs`:
- Around line 409-423: The PrefetchLayer's `seen: Arc<DashSet<Arc<str>>>`
creates a permanent, process-lifetime dedupe that causes unbounded memory growth
and prevents re-prefetching after the cache evicts files; update the
implementation so `seen` is no longer a forever-growing set: replace the DashSet
with a bounded/evicting structure (e.g., an LRU or TTL cache) or tie entries'
lifetime to cache residency via the cache eviction/callback API used elsewhere,
and adjust the logic in methods that insert/check `seen` (search for uses in
PrefetchLayer methods and any code referenced around the current diff) to use
the new eviction-aware API so entries are removed over time or when the
underlying cache evicts the file. Ensure the new structure is thread-safe and
wrapped in Arc like `seen` was, and add tests or a small background cleanup task
if you choose TTL eviction.
- Around line 476-483: The follow-up metadata read logic incorrectly applies the
MAX_FOOTER_READ_SIZE cap and calls pending_footers.remove() too eagerly, which
disables prefetch for large/chunked footers; update the code that decides
whether to inspect a read (the should_check_for_footer(...) call site) to first
check pending_footers for this file and, if an entry exists, bypass the
MAX_FOOTER_READ_SIZE gate so follow-up chunks are processed; change the removal
behavior so pending_footers.remove(...) is only called after the expected
metadata bytes have actually been consumed/handled (i.e., compare buf.len()
against the pending expected remaining footer bytes) rather than on any short
intermediate read; apply the same ordering/behavior corrections to the other
similar block around the should_check_for_footer usage later in the file.
- Around line 685-721: schedule_prefetch_tasks currently spawns an unbounded
detached tokio task per range (using tokio::spawn in schedule_prefetch_tasks and
performing reads via inner.accessor), which can cause huge concurrent fan-out;
add a bounded concurrency mechanism (e.g., a tokio::sync::Semaphore) on
PrefetchInner (e.g., a field like prefetch_semaphore: Arc<Semaphore> configured
globally or per-file) and acquire a permit before doing the read so only N
prefetch reads run concurrently; change schedule_prefetch_tasks to either await
a permit inside each spawned task (or use a small local task queue) and ensure
the permit is held for the duration of the accessor.read call and released when
the read completes (permit guard drop), while preserving the existing
tracker.complete call and metric logging.
In `@crates/icegate-query/src/engine/core.rs`:
- Around line 3-6: The module docstring for QueryEngine advertises
refresh/max-age defaults as 5s/10s but the actual constants
DEFAULT_REFRESH_INTERVAL and DEFAULT_MAX_AGE are 15s/30s; update the top-level
docs to state the real defaults (15s refresh, 30s max age) or change the
constants to match the docs, and ensure any mention of "default 5s/10s" in the
QueryEngine docs is replaced with "default 15s/30s" so documentation and the
DEFAULT_REFRESH_INTERVAL / DEFAULT_MAX_AGE symbols are consistent.
- Around line 133-177: The background task in start_background_refresh currently
captures Arc<Self> (engine) strongly, preventing QueryEngine from ever being
dropped; change the spawn closure to capture a Weak<Self> (Arc::downgrade(self))
and inside the task upgrade the weak ref only when needed (e.g., before calling
Self::build_provider and before checking/sending to engine.provider_tx), exiting
the loop if weak.upgrade() returns None so the task stops when the engine is
dropped; ensure you do not hold the upgraded Arc across the sleep (upgrade, use
to build/send CachedProvider with created_at, then drop the Arc) and keep using
the existing symbols start_background_refresh, build_provider, provider_tx,
refresh_interval, and CachedProvider.
---
Nitpick comments:
In `@config/helm/icegate/templates/deployment-query.yaml`:
- Around line 98-104: When persistence is enabled for query cache ({{
.Values.query.cache.persistence.enabled }}) and the Deployment is scaled to
multiple replicas (e.g. {{ .Values.query.replicaCount }} > 1), a single
ReadWriteOnce PVC ({{ include "icegate.cachePvcName" . }}) will make pods
unschedulable; add a fail-fast guard in the deployment template: check if
.Values.query.cache.persistence.enabled is true AND .Values.query.replicaCount
(or .Values.query.replicaCount | int) > 1, then call Helm's fail function with a
clear message (e.g. "persistence.enabled cannot be true when query.replicaCount
> 1 with RWO PVC") so the chart install/upgrade aborts instead of producing an
unschedulable Deployment.
In `@config/kustomize/overlays/aws-glue/values-icegate.yaml`:
- Around line 46-54: The persistence.storageClass block in values-icegate.yaml
omits fsType; add an explicit fsType (e.g., fsType: "ext4") under the
storageClass for persistence to make the overlay self-documenting and to
override defaults from base values.yaml; update the persistence.storageClass
configuration (type, iops, throughput, encrypted) to include fsType so
production manifests explicitly declare the filesystem type.
In `@crates/icegate-common/Cargo.toml`:
- Around line 46-49: Move the misplaced dependency entry for "futures" out of
the "# Logging" block in Cargo.toml and place it near the other async-related
dependencies (e.g., next to "tokio" and "tokio-util") or under a new "# Async
utilities" section; specifically update the "futures = { workspace = true }"
line so it's grouped with async/runtime deps for clearer organization while
leaving the tracing and tracing-subscriber entries in the Logging section
unchanged.
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 716-718: The loop unnecessarily allocates a Vec via data.to_vec()
before borrowing it; change the call to pass a slice directly to
merged.insert_range by converting the fetched buffer to a byte slice (e.g., use
data.as_ref() or Buffer::chunk()/as_slice depending on OpenDAL version) so that
insert_range(offset, &[u8]) receives a borrowed slice instead of &data.to_vec();
update the loop that iterates over fetched and use the direct slice conversion
for each data item passed to merged.insert_range.
- Around line 1014-1020: The test helper constructs a StorageCache via
foyer::HybridCacheBuilder::new().memory(...).with_weighter(...).storage().build()
which only exercises an in-memory storage path; update the test(s) to also
exercise the disk-backed path by adding a disk device to the builder (e.g., use
the HybridCacheBuilder storage API that accepts a disk device or
storage_with_device before build) or add a separate integration test that
constructs StorageCache with a real temporary disk device and verifies behavior;
target the code around StorageCache / HybridCacheBuilder / .storage().build()
and the weighter using CacheKey and CacheValue to implement this change.
- Around line 815-818: QueueBuf's write method currently pushes every Buffer
into self.buf causing the entire file to be buffered in memory; change it so
writes stream directly to the underlying writer (self.w.write) and only buffer
up to a configurable threshold: track total_buffered_size in QueueBuf, when
total exceeds MAX_BUFFER_SIZE stop appending to self.buf and rely on streaming
to self.w, and on close only populate the cache if total_buffered_size <=
MAX_BUFFER_SIZE (or implement incremental cache population during writes).
Update QueueBuf::write, QueueBuf fields (add total_buffered_size and
max_buffer_size), and QueueBuf::close logic accordingly to avoid buffering
multi-gigabyte files.
- Around line 255-262: The helper u64_to_usize silently truncates on 32-bit
targets which can corrupt offsets; change its signature from const fn
u64_to_usize(v: u64) -> usize to return Option<usize> (e.g., const fn
u64_to_usize(v: u64) -> Option<usize>) and return Some(v as usize) only when v
<= usize::MAX, otherwise None; remove the clippy allow attribute and update all
call sites (functions/methods that call u64_to_usize) to propagate or handle the
None case (return an error or panic with a clear message) so overflow is
detected explicitly instead of being silently truncated.
- Around line 749-758: The current stat cache (inner.stat_cache) only evicts
expired entries on access (the inserted_at/ttl check in the get path), causing
unbounded growth for one-shot keys; add either a background reaper task that
periodically scans inner.stat_cache and removes entries whose
inserted_at.elapsed() >= ttl (e.g., spawn a cleanup task at cache initialization
that iterates and calls inner.stat_cache.remove(key) for expired entries), or
replace inner.stat_cache with a TTL-aware cache like moka and use its time-based
eviction APIs; ensure the cleanup uses the same inserted_at/ttl semantics so
RpStat construction remains unchanged.
- Around line 441-464: KeyLocks currently stores strong Arcs in a DashMap and
never evicts entries except via explicit remove, causing unbounded growth;
change the map to hold Weak<tokio::sync::Mutex<()>> and update KeyLocks::lock to
attempt upgrading the Weak::upgrade(), and if upgrade fails insert a new Arc and
store its Weak, returning the Arc; also add a small cleanup helper (e.g.,
KeyLocks::prune_dead or perform occasional trimming inside lock) that removes
entries whose Weak::upgrade() returns None so stale keys don't accumulate; keep
KeyLocks::remove for explicit deletion but prefer the Weak-based approach to
avoid monotonic memory growth (refer to types KeyLocks, lock, remove, CacheKey,
and DashMap in the diff).
In `@crates/icegate-common/src/storage/icegate_s3.rs`:
- Around line 67-69: In s3_config_parse, avoid double-normalizing boolean
strings: when removing entries like S3_PATH_STYLE_ACCESS (and other keys in the
same block around lines handling path style, assume role, use_tls, etc.), pass
the original string value directly into is_truthy rather than calling
to_lowercase() first; update the call site that currently does
path_style_access.to_lowercase().as_str() to just pass
path_style_access.as_str() (and do the same for the other occurrences) so you
remove the extra temporary allocation while keeping the boolean normalization
inside is_truthy; ensure cfg.enable_virtual_host_style and related assignments
still invert/assign the result as before.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 298-307: The build method currently always constructs
IceGateStorage::S3 using s3_config_parse, which ignores StorageConfig.scheme and
can fail or produce invalid configs; update the StorageFactory::build (fn
build(&self, config: &StorageConfig) -> Result<Arc<dyn Storage>>) to inspect the
scheme from config.props() or config.scheme() and branch: for "s3" parse with
s3_config_parse and return IceGateStorage::S3, for "memory" return the
appropriate IceGateStorage::Memory variant (or other supported variants) using
their respective parsers/constructors, and for unknown schemes return a clear
error result instead of blindly calling s3_config_parse. Ensure to reference
IceGateStorage::S3, s3_config_parse, StorageConfig, and build in your changes.
In `@crates/icegate-query/src/engine/core.rs`:
- Around line 236-260: get_provider can trigger concurrent synchronous calls to
build_provider when the cache is stale; add an async singleflight gate so only
one rebuild runs and others await its result. Introduce a rebuild guard on the
engine (e.g., a tokio::sync::Mutex or a singleflight-like primitive stored on
the struct, name it provider_rebuild_lock or provider_rebuild_inflight) and wrap
the cold/stale path: after checking provider_rx and determining rebuild is
needed, acquire the guard, re-check provider_rx (to avoid TOCTOU), run
build_provider once if still needed, send the CachedProvider via provider_tx,
then release the guard so concurrent callers receive the rebuilt provider
instead of launching duplicate builds.
- Around line 133-178: start_background_refresh currently spawns a new tokio
task every time it is called, which can duplicate work; add a one-shot guard
(e.g., a std::sync::Once or a std::sync::atomic::AtomicBool/AtomicU8 field like
background_refresh_started on the Engine struct) and update
start_background_refresh to check-and-set that guard at the start and return
immediately if already invoked; ensure the guard is initialized with the Engine
and is accessed atomically/thread-safely before cloning Arc<Self> and spawning
the task so only the first caller spawns the refresh loop.
In `@crates/icegate-query/src/engine/provider/scan.rs`:
- Line 196: Replace the hardcoded batch size literal used in
scan_builder.with_batch_size(Some(8192)) by introducing a named constant or
configuration value (e.g., BATCH_SIZE or a runtime-config field) and use that
symbol instead; update the call in the scan construction (the
scan_builder.with_batch_size(...) invocation that produces table_scan) to pass
the new constant or config value so batch-size tuning is centralized and not
hardcoded.
In `@crates/icegate-query/src/engine/provider/schema.rs`:
- Around line 55-75: The current tables_loaded creation uses try_join_all over
table_idents which can spawn unbounded concurrent catalog calls; change this to
a bounded concurrency approach (e.g., convert table_idents.iter() into a
futures::stream::iter and use .map(async move { ... }) then
.buffer_unordered(CONCURRENCY).collect::<Result<Vec<_>, _>>().await or protect
the body with a tokio::sync::Semaphore) so that calls that invoke
IcegateTableProvider::try_new, IcebergStaticTableProvider::try_new_from_table,
and catalog.load_table are limited; introduce a configurable CONCURRENCY
constant or config value and replace try_join_all usage with the bounded
stream/semaphore pattern while preserving the same per-item logic (namespace,
wal_reader, name, provider) and result shape.
In `@crates/icegate-query/src/engine/provider/table.rs`:
- Line 246: Add a focused regression test that supplies an out-of-range
projection index to the projection setup and asserts planning fails
deterministically: create a unit/integration test in the icegate-query crate
that builds the same planner/table builder used in table.rs, call
with_projection_indices(Some(proj.clone())) (or invoke the public planning
entry-point that triggers that call) with a projection Vec containing an index
>= number of table columns, and assert the result is an Err (use an exact error
variant or message check to ensure determinism). Reference the
builder.with_projection_indices call in table.rs and the planner/table provider
setup so the test exercises the same code path and fails reliably on invalid
indices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7eedb55-c676-4b7f-95e9-d491761b7454
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
Cargo.tomlconfig/helm/icegate/templates/_helpers.tplconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/templates/deployment-query.yamlconfig/helm/icegate/templates/pvc-cache.yamlconfig/helm/icegate/templates/storageclass-cache.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/dashboards/Icegate-Query-1773746395045923.jsonconfig/kustomize/base/kustomization.yamlconfig/kustomize/base/values-grafana.yamlconfig/kustomize/base/values-jaeger.yamlconfig/kustomize/base/values-prometheus.yamlconfig/kustomize/overlays/aws-glue/kustomization.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/Cargo.tomlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-common/src/catalog/mod.rscrates/icegate-common/src/lib.rscrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-common/src/storage/mod.rscrates/icegate-common/src/storage/prefetch.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-maintain/src/cli/commands/migrate.rscrates/icegate-maintain/tests/migrate_integration_test.rscrates/icegate-query/Cargo.tomlcrates/icegate-query/benches/common/harness.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/engine/core.rscrates/icegate-query/src/engine/provider/scan.rscrates/icegate-query/src/engine/provider/schema.rscrates/icegate-query/src/engine/provider/table.rscrates/icegate-query/src/infra/metrics.rscrates/icegate-query/src/logql/datafusion/planner_tests.rscrates/icegate-query/tests/loki/harness.rs
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/icegate-common/src/storage/icegate_storage.rs (1)
7-17:⚠️ Potential issue | 🟡 MinorDocumentation still incorrect about OtelMetrics layer position.
Line 15 states "OtelMetrics records metrics for all requests (cache hits + misses)", but the code at lines 129-148 applies layers in this order:
- RetryLayer
- OtelTraceLayer
- OtelMetricsLayer
- CacheLayer
- PrefetchLayer
In OpenDAL's layer model, layers applied later wrap outer, so execution is:
Prefetch → Cache → OtelMetrics → OtelTrace → Retry → S3This means
OtelMetricssits below the cache and only sees cache misses (actual S3 round-trips), not all requests. Update line 15 to:-//! - **`OtelMetrics`** records metrics for all requests (cache hits + misses). +//! - **`OtelMetrics`** records metrics for cache misses (actual S3 round-trips).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 7 - 17, Update the module doc comment so it accurately reflects the layer ordering: change the sentence currently saying "OtelMetrics records metrics for all requests (cache hits + misses)" to state that OtelMetricsLayer sits below CacheLayer and therefore only records metrics for actual S3 round-trips (cache misses); reference the concrete layers OtelMetricsLayer, CacheLayer, PrefetchLayer (as applied in the code around the layer construction using RetryLayer → OtelTraceLayer → OtelMetricsLayer → CacheLayer → PrefetchLayer) to ensure the text matches the implementation.
🧹 Nitpick comments (4)
crates/icegate-query/src/engine/provider/scan.rs (1)
32-34: Consider making Iceberg scan batch size configurable.Hardcoding
8192reduces flexibility across workloads; wiring batch size from runtime/query config would improve operability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/provider/scan.rs` around lines 32 - 34, Replace the hardcoded ICEBERG_SCAN_BATCH_SIZE constant with a configurable value sourced from runtime/query configuration: remove or deprecate the const ICEBERG_SCAN_BATCH_SIZE and instead add a field (e.g., iceberg_scan_batch_size: usize) to the relevant config struct (or retrieve it from an existing QueryConfig), thread that value into the scan initialization code that previously referenced ICEBERG_SCAN_BATCH_SIZE (e.g., the scan builder/constructor or functions in scan.rs), and default to 8192 when the config field is absent; update any callers and tests to accept the injected config value.crates/icegate-common/src/storage/icegate_s3.rs (1)
149-154: Consider documenting the layer-less operator design.The function builds an operator without any layers (as documented), but callers must remember to add retry, tracing, metrics, and caching layers. The
s3_config_buildname doesn't convey this. Consider either:
- Adding a comment at call sites reminding about layering, or
- Renaming to
s3_config_build_bareto make the incomplete nature explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_s3.rs` around lines 149 - 154, The function s3_config_build currently constructs an Operator via Operator::new(builder) with no layers, which callers must augment with retry/tracing/metrics/caching; either add a succinct inline comment at each call site reminding callers to attach layers, or rename the function to s3_config_build_bare (and update all call sites) to make the "layer-less" nature explicit—search for s3_config_build and either insert the comment near each use or perform a rename-refactor to s3_config_build_bare and update imports/usages accordingly.crates/icegate-common/src/storage/prefetch.rs (1)
496-501: Clearingseenentirely loses dedup state and may cause redundant prefetches.The
bounded_seen_insertfunction clears the entireseenset when it exceedsMAX_SEEN_ENTRIES. This prevents unbounded memory growth but causes all previously-seen files to become "unseen" again. If the cache has evicted those files, they will be re-prefetched on next access.Consider using a bounded LRU cache (e.g.,
mini-mokaor a simple evictingDashMapwrapper) that evicts oldest entries individually rather than clearing everything at once. This would maintain better dedup coverage while still bounding memory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/prefetch.rs` around lines 496 - 501, The current bounded_seen_insert clears the entire DashSet when seen.len() > MAX_SEEN_ENTRIES, which loses dedup state; replace this strategy by using a bounded LRU/evicting structure instead of DashSet (e.g., swap DashSet<Arc<str>> for a mini-moka or moka::sync::Cache, or implement a small ring-buffer of keys + DashSet to evict oldest entries) and update bounded_seen_insert to insert the key and evict only the least-recently-used/oldest entry when capacity is exceeded; change references to bounded_seen_insert and the seen field to the chosen bounded cache API so inserts call cache.insert(key) and eviction is automatic (or remove the oldest key explicitly via the ring buffer approach) rather than calling seen.clear().crates/icegate-common/src/storage/icegate_storage.rs (1)
305-317: StorageFactory always builds S3 variant regardless of scheme.
IceGateStorageFactory::buildalways createsIceGateStorage::S3, even ifconfigured_schemeis something other than S3. While the current usage only passes "s3", this could be confusing if someone tries to use a different scheme.Consider adding validation or a comment clarifying that only S3-compatible storage is supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 305 - 317, IceGateStorageFactory::build currently always constructs IceGateStorage::S3 regardless of configured_scheme, which is misleading; update build to validate the configured_scheme (the factory's configured_scheme field and/or config.props()) and either (a) return an Err with a clear message when the scheme is not "s3"/unsupported, or (b) explicitly document with a comment that only S3 is supported; locate the build implementation in IceGateStorageFactory and add the scheme check and error return (or the clarifying comment) so callers are informed when a non-S3 scheme is provided instead of silently creating an S3 variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 75-77: Update the top-of-file comment in Cargo.toml to accurately
reflect upstream tracking: replace the current "Iceberg — fork of
apache/iceberg-rust `develop` branch." wording with a clarified statement that
this repository is a tracked fork of apache/iceberg-rust (specify whether pinned
to a revision or tracking upstream) and note the purpose of the fork (WAL-aware
table scan support) and where maintainer should look for upstream changes (e.g.,
GH-71). Edit the comment text near the existing lines mentioning "pinned to a
specific revision" and "Fork adds WAL-aware table scan support; track upstream
merge progress in GH-71" so the language is consistent and unambiguous about
fork status and upstream reference.
In `@config/helm/icegate/templates/deployment-query.yaml`:
- Around line 106-112: The deployment template always uses the include
"icegate.cachePvcName" for persistentVolumeClaim.claimName and doesn't honor
.Values.query.cache.persistence.existingClaim; change the
persistentVolumeClaim.claimName logic to prefer the user-provided existing claim
when .Values.query.cache.persistence.existingClaim is set (e.g., use that value
if present, otherwise fall back to include "icegate.cachePvcName"), ensuring the
.Values.query.cache.persistence.existingClaim path is referenced in
deployment-query.yaml alongside the existing emptyDir/pvc block so the pod
points at the correct PVC.
- Around line 2-4: The guard currently fails whenever
.Values.query.cache.persistence.enabled is true and .Values.query.replicaCount >
1, but it should only error when the cache is actually enabled; update the
conditional used in the template (the and(...) expression that references
.Values.query.cache.persistence.enabled and .Values.query.replicaCount) to also
require .Values.query.cache.enabled to be true (i.e., include
.Values.query.cache.enabled in the and(...) check) so the failure only occurs
when the cache is enabled and persistence is requested with replicaCount > 1.
---
Duplicate comments:
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 7-17: Update the module doc comment so it accurately reflects the
layer ordering: change the sentence currently saying "OtelMetrics records
metrics for all requests (cache hits + misses)" to state that OtelMetricsLayer
sits below CacheLayer and therefore only records metrics for actual S3
round-trips (cache misses); reference the concrete layers OtelMetricsLayer,
CacheLayer, PrefetchLayer (as applied in the code around the layer construction
using RetryLayer → OtelTraceLayer → OtelMetricsLayer → CacheLayer →
PrefetchLayer) to ensure the text matches the implementation.
---
Nitpick comments:
In `@crates/icegate-common/src/storage/icegate_s3.rs`:
- Around line 149-154: The function s3_config_build currently constructs an
Operator via Operator::new(builder) with no layers, which callers must augment
with retry/tracing/metrics/caching; either add a succinct inline comment at each
call site reminding callers to attach layers, or rename the function to
s3_config_build_bare (and update all call sites) to make the "layer-less" nature
explicit—search for s3_config_build and either insert the comment near each use
or perform a rename-refactor to s3_config_build_bare and update imports/usages
accordingly.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 305-317: IceGateStorageFactory::build currently always constructs
IceGateStorage::S3 regardless of configured_scheme, which is misleading; update
build to validate the configured_scheme (the factory's configured_scheme field
and/or config.props()) and either (a) return an Err with a clear message when
the scheme is not "s3"/unsupported, or (b) explicitly document with a comment
that only S3 is supported; locate the build implementation in
IceGateStorageFactory and add the scheme check and error return (or the
clarifying comment) so callers are informed when a non-S3 scheme is provided
instead of silently creating an S3 variant.
In `@crates/icegate-common/src/storage/prefetch.rs`:
- Around line 496-501: The current bounded_seen_insert clears the entire DashSet
when seen.len() > MAX_SEEN_ENTRIES, which loses dedup state; replace this
strategy by using a bounded LRU/evicting structure instead of DashSet (e.g.,
swap DashSet<Arc<str>> for a mini-moka or moka::sync::Cache, or implement a
small ring-buffer of keys + DashSet to evict oldest entries) and update
bounded_seen_insert to insert the key and evict only the
least-recently-used/oldest entry when capacity is exceeded; change references to
bounded_seen_insert and the seen field to the chosen bounded cache API so
inserts call cache.insert(key) and eviction is automatic (or remove the oldest
key explicitly via the ring buffer approach) rather than calling seen.clear().
In `@crates/icegate-query/src/engine/provider/scan.rs`:
- Around line 32-34: Replace the hardcoded ICEBERG_SCAN_BATCH_SIZE constant with
a configurable value sourced from runtime/query configuration: remove or
deprecate the const ICEBERG_SCAN_BATCH_SIZE and instead add a field (e.g.,
iceberg_scan_batch_size: usize) to the relevant config struct (or retrieve it
from an existing QueryConfig), thread that value into the scan initialization
code that previously referenced ICEBERG_SCAN_BATCH_SIZE (e.g., the scan
builder/constructor or functions in scan.rs), and default to 8192 when the
config field is absent; update any callers and tests to accept the injected
config value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d683f6c-935b-424c-b231-aa8d724aa558
📒 Files selected for processing (15)
Cargo.tomlconfig/helm/icegate/templates/deployment-query.yamlconfig/helm/icegate/templates/storageclass-cache.yamlconfig/helm/icegate/values.yamlconfig/kustomize/overlays/aws-glue/kustomization.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlcrates/icegate-common/Cargo.tomlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-common/src/storage/prefetch.rscrates/icegate-query/src/engine/core.rscrates/icegate-query/src/engine/provider/scan.rscrates/icegate-query/src/engine/provider/schema.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- config/kustomize/overlays/aws-glue/values-icegate.yaml
- config/kustomize/overlays/aws-glue/kustomization.yaml
- crates/icegate-query/src/engine/provider/schema.rs
… and add persistent disk cache support.
…es in storage layer - Extend prefetch logic to include dictionary pages alongside column indexes, offset indexes, and bloom filters. - Introduce `PrefetchRangeSummary` for tracking counts by metadata type. - Enhance telemetry with counters for prefetch ranges by type. - Update dashboard to visualize additional metrics, including dictionary page prefetching and cache gaps per miss.
- Extend `PrefetchConfig` with `prefetch_dictionary_pages` option (default: `false`). - Adjust prefetch logic to fetch dictionary pages when enabled. - Add tests for dictionary prefetching behavior, including default-disabled case.
…ies (lz4_flex, toml).
…ge prefetch, and improve configurability. - Adjusted refresh interval defaults in `QueryEngine` and improved background refresh cleanup logic using weak references. - Introduced `ICEBERG_SCAN_BATCH_SIZE` constant for batch size configurability in Iceberg scans. - Added `max_concurrent_prefetches` with a default of 32 to limit concurrent prefetch tasks. - Ensured bounded memory usage for the `seen` set in prefetch tasks. - Updated Helm values and templates to support dynamic provisioners and validate replica constraints with persistent caches. - Enhanced `PrefetchLayer` integration into storage stack with detailed concurrency controls. - Upgraded Helm chart to version `0.1.0-rc6` and synced dependencies with DataFusion updates.
…g in queue writes
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 (1)
crates/icegate-common/src/storage/builder.rs (1)
27-35:⚠️ Potential issue | 🟠 Major
RetryLayeris documented but not actually applied to the operator stack.Line 5 and line 111 claim
Retryin the stack, but lines 119-123 never applyRetryLayer.RetryLayeris not even imported (line 13). This leaves S3 transient failures unhandled and creates an inconsistency between documented and actual behavior.Proposed fix
-use opendal::layers::{OtelMetricsLayer, OtelTraceLayer}; +use opendal::layers::{OtelMetricsLayer, OtelTraceLayer, RetryLayer};let base = Operator::new(s3) .map_err(|e| CommonError::Config(format!("Failed to build OpenDAL S3 operator: {e}")))? .layer(OtelTraceLayer::default()) + .layer(RetryLayer::new()) .layer(OtelMetricsLayer::builder().register(&meter));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/builder.rs` around lines 27 - 35, The doc mentions a RetryLayer but the operator stack never applies it; modify the S3 store builder (e.g., the function that creates the OpenDAL operator in builder.rs) to import and insert RetryLayer into the operator layering: add use of RetryLayer and apply .layer(RetryLayer::new(...) or appropriate constructor) in the operator build chain (alongside OtelMetricsLayer, FoyerLayer, and OtelTraceLayer) so transient S3 errors are retried; ensure the RetryLayer is configured with sensible retry policy and placed where network calls are effected (before OtelTraceLayer/S3 connector) to match the documented layer order.
🧹 Nitpick comments (5)
crates/icegate-queue/src/config.rs (1)
257-275: Consider using a non-default value to strengthen the builder test.Since
write_retriesdefaults to 5 now, using.with_write_retries(5)doesn't verify that the builder method actually overrides the default—it would pass even if the method were a no-op.♻️ Suggested improvement
let config = QueueConfig::new("s3://bucket/queue") .with_channel_capacity(512) - .with_write_retries(5) + .with_write_retries(7) .with_compression(CompressionCodec::Snappy) .with_max_row_group_size(5000) .with_records_per_flush_multiplier(2) .with_max_bytes_per_flush(32 * 1024 * 1024) .with_flush_interval_ms(1000); assert_eq!(config.common.channel_capacity, 512); assert_eq!(config.common.max_row_group_size, 5000); - assert_eq!(config.write.write_retries, 5); + assert_eq!(config.write.write_retries, 7);🤖 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 257 - 275, The test_builder_pattern uses .with_write_retries(5) which matches the current default so it doesn't verify the builder override; update the test to use a non-default value (e.g., .with_write_retries(3) or another value different from the default) and assert that config.write.write_retries equals that non-default value; modify the assertion in the test function test_builder_pattern and keep references to QueueConfig::new and the with_write_retries builder method to ensure the override is actually exercised.crates/icegate-queue/src/writer.rs (2)
497-505: Conflict branch does an extra probe that is repeated next iterationLine 499 probes for
next, but the next loop iteration probes again at Line 478. Under contention this adds avoidable HEAD calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 497 - 505, The loop currently calls find_next_available_offset(topic, offset + 1) to compute next but then on the next iteration the loop recomputes the same probe; save and reuse that probe result by updating the loop's offset before continuing: after getting next, set offset = next - 1 (or otherwise advance the loop index so the next iteration starts at next) and then continue so you avoid re-probing the same next value; adjust any variable scopes so the saved next is used instead of re-calling find_next_available_offset in the subsequent loop iteration (refer to find_next_available_offset, the local next variable, attempts and offset in this block).
1096-1122: Extend regression test to validate offset continuity after conflict skipThis test checks the first post-conflict write (offset 3), but not that the next write advances correctly. Adding one more write/assertion will guard against offset-state regressions.
✅ Suggested test addition
let offset = writer.write_batch(&"logs".to_string(), test_batch(), None).await.unwrap(); assert_eq!(offset, 3); + let next_offset = writer.write_batch(&"logs".to_string(), test_batch(), None).await.unwrap(); + assert_eq!(next_offset, 4); + // Verify the segment at offset 3 exists let path = Path::from("queue/logs/00000000000000000003.parquet"); assert!(store.head(&path).await.is_ok());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 1096 - 1122, Extend the test_conflict_probe_skips_taken_offsets to assert that after the writer skips taken offsets and writes at 3, a subsequent write advances to 4: after the existing assert_eq!(offset, 3) call, call writer.write_batch(&"logs".to_string(), test_batch(), None).await.unwrap() again and assert the returned offset is 4 and that the corresponding Path ("queue/logs/00000000000000000004.parquet") exists in the store; update references to QueueWriter::write_batch, the local writer variable, and the Path checks accordingly.config/helm/icegate/templates/deployment-query.yaml (1)
14-18: Consider documenting the TODO or creating an issue.The TODO comment indicates uncertainty about the deployment strategy. The
Recreatestrategy is appropriate for RWO PVCs since only one pod can mount the volume at a time, but if horizontal scaling with shared cache is a future goal, this deserves a tracked issue.Would you like me to help open an issue to track exploring StatefulSet or ReadWriteMany storage options for scaling the query component with persistent cache?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/helm/icegate/templates/deployment-query.yaml` around lines 14 - 18, The TODO about deployment safety needs tracking: replace the inline TODO with a short documented note linking to a created issue (or create one) that explores using StatefulSet or ReadWriteMany volumes for .Values.query.cache.persistence.enabled scenarios; keep the current Recreate strategy in the template (strategy: type: Recreate) for RWO PVC compatibility but add a comment referencing the issue number and a brief rationale, and ensure the issue describes evaluating StatefulSet vs Deployment + RWM storage and the scaling implications of .Values.query.cache.enabled / .Values.query.cache.persistence.enabled.config/kustomize/overlays/skaffold/values-icegate.yaml (1)
114-118: Resource limits equal requests - acceptable for local dev.Setting CPU/memory requests equal to limits (no burst capacity) is fine for the skaffold overlay. For production overlays, consider allowing some burst headroom if appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/skaffold/values-icegate.yaml` around lines 114 - 118, The skaffold overlay currently sets resource requests equal to limits (cpu: "1", memory: 2Gi under both requests and limits) which is fine for local dev; leave these values in config/kustomize/overlays/skaffold/values-icegate.yaml as-is, but for production overlays update the production overlay’s requests to be lower than limits (e.g., reduce requests for cpu and memory while keeping limits at desired caps) so pods can burst up to the limits; look for the cpu, memory, requests and limits entries in your production overlay manifests and adjust 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 `@crates/icegate-common/src/storage/prefetch.rs`:
- Around line 739-744: The prefetch task calls inner.accessor.read(&path,
OpRead::default().with_range(range)) but drops the returned Reader, so the cache
layer never sees consumed bytes; change the flow to await and consume the reader
(e.g., call the reader's read_all()/read_to_end-equivalent and await it) before
calling inner.tracker.complete(&path, offset, length) so the cache warm-up code
(the cache-populating path referenced around lines 700-706) runs; ensure errors
from the read are handled/propagated appropriately so complete() still runs in
the finally-like path.
- Around line 445-446: The semaphore is created with
self.config.max_concurrent_prefetches which may be 0, causing acquire() to await
forever and preventing tracker.complete() from running; fix by guarding when
constructing prefetch_semaphore (or before acquire()) to ensure at least one
permit—e.g., use max(1, self.config.max_concurrent_prefetches) or
early-return/skip prefetch logic when the config is 0 so acquire() cannot block
indefinitely; update the construction of prefetch_semaphore (and any code paths
that call acquire()) to use this nonzero-guarded value so tracker.complete() is
always reachable.
In `@crates/icegate-queue/src/writer.rs`:
- Around line 452-463: In find_next_available_offset, avoid regressing local
state and the infinite loop at u64::MAX: when you find a free segment using
SegmentId::new(topic, offset) call set_offset with the next free offset
(offset.saturating_add(1) or checked_add(1)) so that next_offset() reflects the
next writable slot, and ensure you handle the u64::MAX case by returning an
error (or propagating one) instead of looping forever when offset == u64::MAX
and the segment exists; update callers/semantics accordingly so set_offset and
next_offset remain consistent.
---
Outside diff comments:
In `@crates/icegate-common/src/storage/builder.rs`:
- Around line 27-35: The doc mentions a RetryLayer but the operator stack never
applies it; modify the S3 store builder (e.g., the function that creates the
OpenDAL operator in builder.rs) to import and insert RetryLayer into the
operator layering: add use of RetryLayer and apply .layer(RetryLayer::new(...)
or appropriate constructor) in the operator build chain (alongside
OtelMetricsLayer, FoyerLayer, and OtelTraceLayer) so transient S3 errors are
retried; ensure the RetryLayer is configured with sensible retry policy and
placed where network calls are effected (before OtelTraceLayer/S3 connector) to
match the documented layer order.
---
Nitpick comments:
In `@config/helm/icegate/templates/deployment-query.yaml`:
- Around line 14-18: The TODO about deployment safety needs tracking: replace
the inline TODO with a short documented note linking to a created issue (or
create one) that explores using StatefulSet or ReadWriteMany volumes for
.Values.query.cache.persistence.enabled scenarios; keep the current Recreate
strategy in the template (strategy: type: Recreate) for RWO PVC compatibility
but add a comment referencing the issue number and a brief rationale, and ensure
the issue describes evaluating StatefulSet vs Deployment + RWM storage and the
scaling implications of .Values.query.cache.enabled /
.Values.query.cache.persistence.enabled.
In `@config/kustomize/overlays/skaffold/values-icegate.yaml`:
- Around line 114-118: The skaffold overlay currently sets resource requests
equal to limits (cpu: "1", memory: 2Gi under both requests and limits) which is
fine for local dev; leave these values in
config/kustomize/overlays/skaffold/values-icegate.yaml as-is, but for production
overlays update the production overlay’s requests to be lower than limits (e.g.,
reduce requests for cpu and memory while keeping limits at desired caps) so pods
can burst up to the limits; look for the cpu, memory, requests and limits
entries in your production overlay manifests and adjust accordingly.
In `@crates/icegate-queue/src/config.rs`:
- Around line 257-275: The test_builder_pattern uses .with_write_retries(5)
which matches the current default so it doesn't verify the builder override;
update the test to use a non-default value (e.g., .with_write_retries(3) or
another value different from the default) and assert that
config.write.write_retries equals that non-default value; modify the assertion
in the test function test_builder_pattern and keep references to
QueueConfig::new and the with_write_retries builder method to ensure the
override is actually exercised.
In `@crates/icegate-queue/src/writer.rs`:
- Around line 497-505: The loop currently calls
find_next_available_offset(topic, offset + 1) to compute next but then on the
next iteration the loop recomputes the same probe; save and reuse that probe
result by updating the loop's offset before continuing: after getting next, set
offset = next - 1 (or otherwise advance the loop index so the next iteration
starts at next) and then continue so you avoid re-probing the same next value;
adjust any variable scopes so the saved next is used instead of re-calling
find_next_available_offset in the subsequent loop iteration (refer to
find_next_available_offset, the local next variable, attempts and offset in this
block).
- Around line 1096-1122: Extend the test_conflict_probe_skips_taken_offsets to
assert that after the writer skips taken offsets and writes at 3, a subsequent
write advances to 4: after the existing assert_eq!(offset, 3) call, call
writer.write_batch(&"logs".to_string(), test_batch(), None).await.unwrap() again
and assert the returned offset is 4 and that the corresponding Path
("queue/logs/00000000000000000004.parquet") exists in the store; update
references to QueueWriter::write_batch, the local writer variable, and the Path
checks accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21fba0ad-9ce0-40a7-9952-4345e3fecef6
📒 Files selected for processing (12)
Cargo.tomlconfig/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/templates/deployment-ingest.yamlconfig/helm/icegate/templates/deployment-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-common/src/storage/prefetch.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-queue/src/config.rscrates/icegate-queue/src/writer.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/icegate-ingest/src/cli/commands/run.rs
- crates/icegate-common/src/storage/icegate_storage.rs
- Remove retry layer from storage stack. - Ensure concurrency limits for prefetch are at least 1. - Handle offset overflow in queue writes with an explicit error. - Adjust queue write retries default from 5 to 3.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/icegate-ingest/src/cli/commands/run.rs (1)
245-281:⚠️ Potential issue | 🟠 MajorAvoid constructing
IoHandletwice; reuse one handle end-to-end.Line [245] and Line [280] build separate handles, but only one is explicitly closed on Line [386]. This can duplicate cache resources and leave one handle’s async cleanup path unexecuted.
Proposed fix
- let io_cache = IoHandle::from_config(config.catalog.cache.as_ref(), config.catalog.prefetch.clone()).await?; + let io_handle = IoHandle::from_config(config.catalog.cache.as_ref(), config.catalog.prefetch.clone()).await?; @@ let (store, normalized_path) = create_object_store( &queue_config.common.base_path, Some(&config.storage.backend), - io_cache.cache(), - io_cache.prefetch(), + io_handle.cache(), + io_handle.prefetch(), None, )?; @@ - let io_cache = IoHandle::from_config(config.catalog.cache.as_ref(), config.catalog.prefetch.clone()).await?; - let catalog = CatalogBuilder::from_config(&config.catalog, &io_cache).await?; + let catalog = CatalogBuilder::from_config(&config.catalog, &io_handle).await?; @@ - io_cache.close().await; + io_handle.close().await;Also applies to: 386-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-ingest/src/cli/commands/run.rs` around lines 245 - 281, The code constructs IoHandle twice (IoHandle::from_config) creating duplicated cache resources; fix by removing the second call and reusing the original io_cache for subsequent operations (pass io_cache.cache()/io_cache.prefetch() to create_object_store and then reuse the same io_cache when calling CatalogBuilder::from_config), avoid shadowing the io_cache variable, and ensure the single IoHandle is the one closed/awaited in the cleanup path so only one async cleanup runs.
🧹 Nitpick comments (5)
config/helm/icegate/templates/deployment-query.yaml (1)
14-18: Consider addressing the TODO in a follow-up.The
Recreatestrategy is necessary for RWO PVCs, but as the TODO notes, this could be improved. A StatefulSet might be more appropriate if PVC-backed caching becomes the primary deployment pattern, as it provides better volume affinity guarantees.Would you like me to open an issue to track evaluating StatefulSet for PVC-backed query deployments?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/helm/icegate/templates/deployment-query.yaml` around lines 14 - 18, The TODO about making the deployment safer should be tracked: create a follow-up issue to evaluate using a StatefulSet for PVC-backed query caching (when .Values.query.cache.enabled and .Values.query.cache.persistence.enabled are true) and update the TODO in the template near the strategy: type: Recreate block to reference that issue (include the issue ID/URL) and a short rationale that Recreate is retained for RWO PVC compatibility until the StatefulSet evaluation is complete. Ensure the comment references the conditional (.Values.query.cache.persistence.enabled) and the current strategy: type: Recreate so future reviewers can find and close the follow-up once implemented.config/kustomize/overlays/skaffold/values-icegate.yaml (2)
79-84: Consider addingstatTtlSecsto ingest cache configuration.The
query.cacheblock includesstatTtlSecs: 300(line 43), but theingest.cacheblock does not. If stat TTL is relevant for the ingest cache as well, consider adding it for consistency. If ingest intentionally uses a different default or doesn't need stat caching, this can be ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/skaffold/values-icegate.yaml` around lines 79 - 84, The ingest cache configuration is missing statTtlSecs like the query.cache block; add a statTtlSecs entry (e.g., statTtlSecs: 300) to the ingest.cache configuration so ingest.cache uses the same stat TTL as query.cache (update the ingest.cache/cache block in this file and ensure the key name is statTtlSecs to match query.cache).
112-118: Resource requests equal limits - Guaranteed QoS with no burstability.Setting
requestsequal tolimitscreates a Kubernetes Guaranteed QoS class. This provides predictable scheduling but means:
- No CPU bursting above 1 core during load spikes
- Immediate OOM kill if memory exceeds 2Gi
With the new cache/prefetch functionality enabled for ingest, verify that 2Gi is sufficient headroom. If ingest memory usage is variable (e.g., during cache population or batch prefetch), consider allowing some burst capacity by setting limits slightly higher than requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/skaffold/values-icegate.yaml` around lines 112 - 118, The resources block currently sets requests equal to limits (cpu: "1", memory: 2Gi) which forces Guaranteed QoS and prevents bursting; update the resources so requests reflect the expected baseline (e.g., cpu request "1", memory request 2Gi) but set higher limits to allow burst capacity (e.g., cpu limit > request such as "2" and memory limit > request such as 3Gi or another value based on ingest profiling), and re-evaluate memory headroom for cache/prefetch to pick appropriate limit values; modify the resources.requests and resources.limits entries accordingly.crates/icegate-common/src/storage/icegate_storage.rs (1)
120-148: Comment at line 124 is incorrect.The comment states "OtelMetrics sees all requests (cache hits + misses)" but the layer application order (OtelTrace → OtelMetrics → Cache → Prefetch) means OtelMetrics is below the cache in the execution stack, so it only sees cache misses (S3 round-trips). The documentation at lines 14-16 correctly states "OtelMetrics records metrics for S3 round-trips (cache misses only)".
Suggested fix
// Layer stack (applied bottom-up, executed top-down): // Retry → OtelTrace → OtelMetrics → [FoyerCache] → [Prefetch] // - // OtelTrace sits below OtelMetrics; cache sits outermost (before Prefetch). - // OtelMetrics sees all requests (cache hits + misses). - // OtelTrace only traces actual S3 round-trips (below cache). + // Cache sits outermost (before Prefetch), metrics/trace sit below cache. + // Both OtelMetrics and OtelTrace only observe S3 round-trips (cache misses).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 120 - 148, Update the incorrect inline comment that claims "OtelMetrics sees all requests (cache hits + misses)" to reflect the actual layer order implemented by the operator stacking (bare.layer(OtelTraceLayer::default()) then optionally .layer(OtelMetricsLayer::builder().register(m)) then .layer(CacheLayer::new(...)) and PrefetchLayer outermost) — change the wording to state that OtelMetricsLayer is below the cache and therefore only records S3 round-trips (cache misses), while OtelTraceLayer likewise only traces actual S3 round-trips below the cache; reference the symbols OtelTraceLayer, OtelMetricsLayer, CacheLayer and PrefetchLayer when editing the comment so it matches the implemented operator layering.crates/icegate-common/src/storage/builder.rs (1)
31-35: Documentation layer order is incorrect.The docstring shows
OtelMetricsLayer→FoyerLayer→OtelTraceLayer→ S3, but the code (lines 119-139) constructs:Prefetch→Cache→OtelMetrics→OtelTrace→ S3 (execution order). The comment at lines 113-114 correctly states metrics/trace observe only S3 round-trips (below cache), which matches the code.Suggested fix
/// 1. **`OtelMetricsLayer`** — `OpenTelemetry` storage metrics (sees cache hits) -/// 2. **`FoyerLayer`** — shared hybrid cache (if `cache` is provided) -/// 3. **`OtelTraceLayer`** — `OpenTelemetry` distributed tracing (S3 calls only) +/// 2. **`PrefetchLayer`** — Parquet column-chunk prefetch (if `prefetch` is enabled) +/// 3. **`FoyerLayer`** — shared hybrid cache (if `cache` is provided) +/// 4. **`OtelMetricsLayer`** — `OpenTelemetry` storage metrics (S3 calls only) +/// 5. **`OtelTraceLayer`** — `OpenTelemetry` distributed tracing (S3 calls only)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/builder.rs` around lines 31 - 35, The docstring order is wrong—update the documentation so it reflects the actual layer construction used by the builder: Prefetch → FoyerLayer (Cache) → OtelMetricsLayer → OtelTraceLayer → S3, and explicitly note that OtelMetricsLayer and OtelTraceLayer observe only S3 round-trips (i.e., they are below the cache layer); reference the builder symbols Prefetch, Cache/FoyerLayer, OtelMetrics (OtelMetricsLayer), OtelTrace (OtelTraceLayer) and S3 when making the wording change.
🤖 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/icegate_s3.rs`:
- Around line 80-95: When parsing SSE settings in icegate_s3.rs, fail fast if
sse_type == "custom" but s3_sse_key is None: check the s3_sse_key value (the
result of m.remove(S3_SSE_KEY)) before setting
cfg.server_side_encryption_customer_algorithm/key/md5 and if it's None return an
appropriate parse error (or Err) instead of accepting the config; only assign
cfg.server_side_encryption_customer_key/_md5 when s3_sse_key is Some(...) and
ensure the error includes context that SSE-C (custom) requires a customer key.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 7-17: The docs claim a layer stack including Retry but the
implemented stack (uses OtelTraceLayer, OtelMetricsLayer, CacheLayer,
PrefetchLayer) omits RetryLayer; either remove Retry from the README/comment or
actually insert RetryLayer into the constructed stack and remove the “callers
are responsible for adding retry” note. Fix by editing the layer-construction
code that composes OtelTraceLayer/OtelMetricsLayer/CacheLayer/PrefetchLayer (the
function that builds the storage/middleware stack, and the comment in
icegate_s3.rs referencing caller responsibility) to include RetryLayer with
appropriate position and configuration, or alternatively update the top-of-file
documentation text to omit “Retry → S3” and adjust the description of retry
behavior to state it’s a caller responsibility.
In `@crates/icegate-query/src/engine/core.rs`:
- Around line 242-269: get_provider currently allows concurrent callers to all
call build_provider when cache is cold/stale causing a stampede; add a
single-flight rebuild guard (e.g., an async lock field like rebuild_lock:
tokio::sync::Mutex<()>) around the synchronous rebuild path in get_provider so
only one task runs build_provider at a time. Specifically: before calling
Self::build_provider, acquire the async lock; once the lock is held, re-check
the cached provider via self.provider_rx.borrow().clone() and return it if
another task already refreshed it; otherwise call build_provider, send the new
CachedProvider via self.provider_tx, then release the lock. Update struct to
include the lock and reference it by name in get_provider to prevent parallel
rebuilds while keeping the existing
provider_rx/provider_tx/CachedProvider/max_age logic.
In `@crates/icegate-queue/src/writer.rs`:
- Around line 501-509: The code calls self.find_next_available_offset(topic,
offset + 1) which can panic/wrap when offset == u64::MAX; change this to use
offset.checked_add(1) and handle the None case consistently (similar to the
checked_add usage at line 461): compute next_arg =
offset.checked_add(1).ok_or_else(|| /* appropriate error */)? or otherwise skip
probing and return a clear error, then call find_next_available_offset(topic,
next_arg); update the debug log references (conflicted_offset, next_offset,
attempt) accordingly and ensure the function/method find_next_available_offset
is invoked only with a valid u64.
---
Outside diff comments:
In `@crates/icegate-ingest/src/cli/commands/run.rs`:
- Around line 245-281: The code constructs IoHandle twice
(IoHandle::from_config) creating duplicated cache resources; fix by removing the
second call and reusing the original io_cache for subsequent operations (pass
io_cache.cache()/io_cache.prefetch() to create_object_store and then reuse the
same io_cache when calling CatalogBuilder::from_config), avoid shadowing the
io_cache variable, and ensure the single IoHandle is the one closed/awaited in
the cleanup path so only one async cleanup runs.
---
Nitpick comments:
In `@config/helm/icegate/templates/deployment-query.yaml`:
- Around line 14-18: The TODO about making the deployment safer should be
tracked: create a follow-up issue to evaluate using a StatefulSet for PVC-backed
query caching (when .Values.query.cache.enabled and
.Values.query.cache.persistence.enabled are true) and update the TODO in the
template near the strategy: type: Recreate block to reference that issue
(include the issue ID/URL) and a short rationale that Recreate is retained for
RWO PVC compatibility until the StatefulSet evaluation is complete. Ensure the
comment references the conditional (.Values.query.cache.persistence.enabled) and
the current strategy: type: Recreate so future reviewers can find and close the
follow-up once implemented.
In `@config/kustomize/overlays/skaffold/values-icegate.yaml`:
- Around line 79-84: The ingest cache configuration is missing statTtlSecs like
the query.cache block; add a statTtlSecs entry (e.g., statTtlSecs: 300) to the
ingest.cache configuration so ingest.cache uses the same stat TTL as query.cache
(update the ingest.cache/cache block in this file and ensure the key name is
statTtlSecs to match query.cache).
- Around line 112-118: The resources block currently sets requests equal to
limits (cpu: "1", memory: 2Gi) which forces Guaranteed QoS and prevents
bursting; update the resources so requests reflect the expected baseline (e.g.,
cpu request "1", memory request 2Gi) but set higher limits to allow burst
capacity (e.g., cpu limit > request such as "2" and memory limit > request such
as 3Gi or another value based on ingest profiling), and re-evaluate memory
headroom for cache/prefetch to pick appropriate limit values; modify the
resources.requests and resources.limits entries accordingly.
In `@crates/icegate-common/src/storage/builder.rs`:
- Around line 31-35: The docstring order is wrong—update the documentation so it
reflects the actual layer construction used by the builder: Prefetch →
FoyerLayer (Cache) → OtelMetricsLayer → OtelTraceLayer → S3, and explicitly note
that OtelMetricsLayer and OtelTraceLayer observe only S3 round-trips (i.e., they
are below the cache layer); reference the builder symbols Prefetch,
Cache/FoyerLayer, OtelMetrics (OtelMetricsLayer), OtelTrace (OtelTraceLayer) and
S3 when making the wording change.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 120-148: Update the incorrect inline comment that claims
"OtelMetrics sees all requests (cache hits + misses)" to reflect the actual
layer order implemented by the operator stacking
(bare.layer(OtelTraceLayer::default()) then optionally
.layer(OtelMetricsLayer::builder().register(m)) then
.layer(CacheLayer::new(...)) and PrefetchLayer outermost) — change the wording
to state that OtelMetricsLayer is below the cache and therefore only records S3
round-trips (cache misses), while OtelTraceLayer likewise only traces actual S3
round-trips below the cache; reference the symbols OtelTraceLayer,
OtelMetricsLayer, CacheLayer and PrefetchLayer when editing the comment so it
matches the implemented operator layering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 536b2be4-92d0-4604-9597-8705c0fb1e61
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
Cargo.tomlconfig/helm/icegate/templates/_helpers.tplconfig/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/templates/deployment-ingest.yamlconfig/helm/icegate/templates/deployment-query.yamlconfig/helm/icegate/templates/pvc-cache.yamlconfig/helm/icegate/templates/storageclass-cache.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/dashboards/Icegate-Query-1773746395045923.jsonconfig/kustomize/base/kustomization.yamlconfig/kustomize/base/values-grafana.yamlconfig/kustomize/base/values-jaeger.yamlconfig/kustomize/base/values-prometheus.yamlconfig/kustomize/overlays/aws-glue/kustomization.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/Cargo.tomlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-common/src/catalog/mod.rscrates/icegate-common/src/lib.rscrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-common/src/storage/mod.rscrates/icegate-common/src/storage/prefetch.rscrates/icegate-ingest/Cargo.tomlcrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-maintain/src/cli/commands/migrate.rscrates/icegate-maintain/tests/migrate_integration_test.rscrates/icegate-query/Cargo.tomlcrates/icegate-query/benches/common/harness.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/engine/core.rscrates/icegate-query/src/engine/provider/scan.rscrates/icegate-query/src/engine/provider/schema.rscrates/icegate-query/src/engine/provider/table.rscrates/icegate-query/src/infra/metrics.rscrates/icegate-query/src/logql/datafusion/planner_tests.rscrates/icegate-query/tests/loki/harness.rscrates/icegate-queue/src/config.rscrates/icegate-queue/src/writer.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/icegate-common/src/catalog/mod.rs
- crates/icegate-query/src/logql/datafusion/planner_tests.rs
- crates/icegate-common/src/catalog/config.rs
- crates/icegate-queue/src/config.rs
- crates/icegate-maintain/src/cli/commands/migrate.rs
- crates/icegate-maintain/tests/migrate_integration_test.rs
- crates/icegate-query/src/engine/provider/table.rs
- crates/icegate-query/src/infra/metrics.rs
- crates/icegate-query/benches/common/harness.rs
- config/kustomize/base/kustomization.yaml
- crates/icegate-query/src/engine/provider/schema.rs
- config/kustomize/base/values-prometheus.yaml
- config/kustomize/base/values-grafana.yaml
- crates/icegate-common/src/storage/mod.rs
- crates/icegate-common/Cargo.toml
- config/helm/icegate/templates/_helpers.tpl
- config/kustomize/base/values-jaeger.yaml
- crates/icegate-query/Cargo.toml
- Remove retry layer from storage stack. - Add single-flight locking for synchronous rebuilds in `QueryEngine`. - Improve concurrency handling and error messaging for offset handling in queues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/icegate-queue/src/writer.rs (1)
480-483:⚠️ Potential issue | 🟠 MajorAvoid reintroducing the offset-state regression on the happy path.
At Line 482,
next_offset()has already advanced the cursor. Callingfind_next_available_offset()immediately after that writes the map back to the current slot, so successful writes leaveoffsetsone behind and every steady-state append pays an extraHEAD. TheAlreadyExistsbranch at Line 491 is already the right place to probe forward.💡 Proposed fix
loop { let offset = self.next_offset(topic).await; - // Check the offset is free before writing, to avoid race conditions. - let offset = self.find_next_available_offset(topic, offset).await?; let segment_id = SegmentId::new(topic, offset); let attempt = attempts + 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 480 - 483, The code advances the cursor with next_offset() then immediately calls find_next_available_offset(), which rewrites offsets and causes the off-by-one regression; instead stop calling find_next_available_offset() on the happy path—use the offset returned from next_offset() to build SegmentId::new(topic, offset) and perform the write, and only invoke find_next_available_offset() inside the AlreadyExists handling branch (the branch that currently probes forward) to resolve collisions and update the offsets map; update uses of SegmentId, next_offset, and find_next_available_offset accordingly so successful appends do not move the offsets map backward.
🧹 Nitpick comments (1)
crates/icegate-common/src/storage/icegate_storage.rs (1)
303-315: Consider supporting Memory backend in factory.The
buildmethod always creates anIceGateStorage::S3variant regardless of theStorageConfigcontents. If a Memory backend is ever needed via the factory interface, this would require changes. If S3-only is intentional, a brief doc comment clarifying this would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 303 - 315, The factory currently always constructs an IceGateStorage::S3 in IceGateStorageFactory::build regardless of StorageConfig; update build to inspect the incoming StorageConfig (e.g. a scheme/kind property on StorageConfig or config.props()) and branch to construct the appropriate variant (e.g. IceGateStorage::Memory when requested) or, if Memory support is not desired, add a clear doc comment on IceGateStorageFactory::build stating it intentionally only builds S3 backends; ensure the implementation references the StorageConfig field used for selection and preserves existing S3 construction (s3_config_parse(...) -> IceGateStorage::S3) while adding the Memory branch (or the explanatory doc) so callers and reviewers can see the intended 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-ingest/src/cli/commands/run.rs`:
- Around line 248-255: The comment above the create_object_store call is
misleading: it says "no read cache or stat cache needed, pass None" but the call
passes io_cache.cache() and io_cache.prefetch(); either update the comment to
state that read cache and prefetch are intentionally supplied via
io_cache.cache() and io_cache.prefetch(), or change the second and third
arguments to None to disable caching for the ingest write path; refer to the
create_object_store invocation and the io_cache.cache() / io_cache.prefetch()
calls when making the change.
---
Duplicate comments:
In `@crates/icegate-queue/src/writer.rs`:
- Around line 480-483: The code advances the cursor with next_offset() then
immediately calls find_next_available_offset(), which rewrites offsets and
causes the off-by-one regression; instead stop calling
find_next_available_offset() on the happy path—use the offset returned from
next_offset() to build SegmentId::new(topic, offset) and perform the write, and
only invoke find_next_available_offset() inside the AlreadyExists handling
branch (the branch that currently probes forward) to resolve collisions and
update the offsets map; update uses of SegmentId, next_offset, and
find_next_available_offset accordingly so successful appends do not move the
offsets map backward.
---
Nitpick comments:
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 303-315: The factory currently always constructs an
IceGateStorage::S3 in IceGateStorageFactory::build regardless of StorageConfig;
update build to inspect the incoming StorageConfig (e.g. a scheme/kind property
on StorageConfig or config.props()) and branch to construct the appropriate
variant (e.g. IceGateStorage::Memory when requested) or, if Memory support is
not desired, add a clear doc comment on IceGateStorageFactory::build stating it
intentionally only builds S3 backends; ensure the implementation references the
StorageConfig field used for selection and preserves existing S3 construction
(s3_config_parse(...) -> IceGateStorage::S3) while adding the Memory branch (or
the explanatory doc) so callers and reviewers can see the intended behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e6ddeae-2bc5-4777-837d-345d844535f8
📒 Files selected for processing (6)
crates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-query/src/engine/core.rscrates/icegate-queue/src/writer.rs
- Introduced `statTtlSecs` for caching stat/HEAD responses in IO cache configuration. - Updated Helm and Kustomize templates to include `statTtlSecs` parameter. - Integrated `statTtlSecs` into object store creation for enhanced cache control.
…source allocation and caching configuration
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
config/kustomize/overlays/skaffold/values-icegate.yaml (1)
86-87: The prefetch section is correctly absent whenenabled: falseis set, but consider explicit defaults for clarity.The template (line 22 of configmap-ingest.yaml) conditionally renders the prefetch block only when
.Values.ingest.prefetch.enabledistrue. Settingenabled: falseresults in the entire prefetch section being absent from the rendered config.In Rust, the
CatalogConfigdeserializes this absence asOption<PrefetchConfig>::None, which is interpreted as disabled at runtime—so prefetch is correctly disabled. However, there's a fragility: thePrefetchConfig.enabledfield has#[serde(default = "default_enabled")], which defaults totrue. If the prefetch section is ever present in YAML without an explicitenabledfield, it would be enabled by default, which may not match the intended behavior.Suggested fix: In the Helm template, explicitly emit
enabled: falsewhen prefetch is disabled, or ensure the template always includes an explicitenabledfield to avoid relying on serde defaults:- {{- if .Values.ingest.prefetch.enabled }} + prefetch: + enabled: {{ .Values.ingest.prefetch.enabled | default false }}This makes the configuration intent explicit at both the template and runtime levels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/skaffold/values-icegate.yaml` around lines 86 - 87, The prefetch YAML block is omitted when .Values.ingest.prefetch.enabled is false which leads to Option<PrefetchConfig>::None being interpreted as disabled, but PrefetchConfig's enabled field uses #[serde(default = "default_enabled")] (default_enabled) which returns true if the block appears without an explicit enabled field; update the template configmap-ingest.yaml to always emit an explicit enabled: false when prefetch is disabled (or otherwise ensure the template always writes an explicit enabled field under prefetch) so the generated manifest and the deserialized CatalogConfig/PrefetchConfig reflect the intended disabled state without relying on serde defaults.crates/icegate-queue/src/writer.rs (1)
564-592: Consider extracting theAlreadyExistscheck to avoid duplicate match.The
matches!(e, object_store::Error::AlreadyExists { .. })pattern is checked twice: once for log-level selection (line 564) and again for error conversion (line 579). Extracting this to a local variable would reduce duplication.♻️ Proposed refactor
let result = put_result.map_err(|e| { + let is_conflict = matches!(e, object_store::Error::AlreadyExists { .. }); - if matches!(e, object_store::Error::AlreadyExists { .. }) { + if is_conflict { debug!( reason = object_store_error_reason(&e), elapsed_ms, error = %e, "store.put_opts conflict (expected in multi-replica)" ); } else { warn!( reason = object_store_error_reason(&e), elapsed_ms, error = %e, "store.put_opts failed" ); } - if matches!(e, object_store::Error::AlreadyExists { .. }) { + if is_conflict { debug!("Segment already exists, skipping write (offset {})", segment_id.offset); QueueError::AlreadyExists { topic: segment_id.topic.clone(), offset: segment_id.offset, } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 564 - 592, Extract the duplicate matches!(e, object_store::Error::AlreadyExists { .. }) into a local boolean (e.g., already_exists) before the logging and error mapping so you only evaluate the pattern once; then use that variable in the existing debug!/warn! branch selection and in the error conversion branch that returns QueueError::AlreadyExists or QueueError::Write, referencing the existing variables e and segment_id to keep the same log fields and returned error variants.
🤖 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/builder.rs`:
- Around line 32-35: The doc comment listing storage layering is inconsistent
with the "desired order" block inside the builder: unify them to a single
canonical order (include PrefetchLayer if configured) so both places match;
update the inline comment at the top (the list with PrefetchLayer, FoyerLayer,
OtelMetricsLayer, OtelTraceLayer) and the mid-function "desired order" block in
the same file (builder.rs) to reflect the exact stack used by the builder
functions that compose PrefetchLayer, FoyerLayer (cache), OtelMetricsLayer, and
OtelTraceLayer in the same sequence.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 108-117: The validation currently only accepts S3 URLs that start
with "{scheme}://{bucket}/" and rejects "{scheme}://{bucket}" (no trailing
slash); update the logic in create_operator / the block using variables prefix,
configured_scheme, and op_info.name() so it also treats an exact match of
format!("{configured_scheme}://{bucket}") as a valid URL mapping to an empty
relative path. Concretely, check for an exact equality to
format!("{configured_scheme}://{}", op_info.name()) and set relative = "" in
that case, otherwise keep the existing starts_with(prefix) branch to slice after
the trailing slash; leave the Err(Error::new(...)) path for other mismatches
(optionally update the error message to mention both accepted forms).
In `@crates/icegate-ingest/src/shift/parquet_meta_reader.rs`:
- Around line 364-385: The current update_lower_bound and update_upper_bound use
direct PartialOrd which silently fails when either side is NaN; change both
functions to explicitly handle float Datums: when datum is a floating NaN, do
not insert/update bounds (i.e., ignore NaN statistics); when the existing bound
is a floating NaN and the incoming datum is a non-NaN value, replace the
existing NaN with the non-NaN datum; otherwise use the existing comparison
logic. Update the implementations of update_lower_bound and update_upper_bound
to pattern-match Datum variants for floats and use is_nan checks before applying
the > / < comparisons so NaNs do not block valid updates and are never stored as
bounds.
In `@crates/icegate-query/src/engine/core.rs`:
- Around line 136-189: start_background_refresh currently spawns a new tokio
task every call causing duplicate refresh work; make it idempotent by ensuring
only one background task can be spawned: add a shared atomic/flag or an
Option<JoinHandle> stored on Engine (e.g., a AtomicBool like
background_refresh_started or background_refresh_handle) and atomically
check-and-set it at the start of start_background_refresh(); if the flag is
already set return early, otherwise set it and spawn the task as existing code
does, and when the task exits clear/reset the flag/handle so it can be restarted
if desired; reference start_background_refresh, provider_tx, and the spawned
task logic when applying the change.
---
Nitpick comments:
In `@config/kustomize/overlays/skaffold/values-icegate.yaml`:
- Around line 86-87: The prefetch YAML block is omitted when
.Values.ingest.prefetch.enabled is false which leads to
Option<PrefetchConfig>::None being interpreted as disabled, but PrefetchConfig's
enabled field uses #[serde(default = "default_enabled")] (default_enabled) which
returns true if the block appears without an explicit enabled field; update the
template configmap-ingest.yaml to always emit an explicit enabled: false when
prefetch is disabled (or otherwise ensure the template always writes an explicit
enabled field under prefetch) so the generated manifest and the deserialized
CatalogConfig/PrefetchConfig reflect the intended disabled state without relying
on serde defaults.
In `@crates/icegate-queue/src/writer.rs`:
- Around line 564-592: Extract the duplicate matches!(e,
object_store::Error::AlreadyExists { .. }) into a local boolean (e.g.,
already_exists) before the logging and error mapping so you only evaluate the
pattern once; then use that variable in the existing debug!/warn! branch
selection and in the error conversion branch that returns
QueueError::AlreadyExists or QueueError::Write, referencing the existing
variables e and segment_id to keep the same log fields and returned error
variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86ca6b72-50e6-4342-b035-f7aa8f7a27a0
📒 Files selected for processing (11)
config/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/values-jaeger.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-ingest/src/shift/parquet_meta_reader.rscrates/icegate-query/src/engine/core.rscrates/icegate-queue/src/writer.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- config/helm/icegate/values.yaml
- config/helm/icegate/templates/configmap-ingest.yaml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
crates/icegate-common/src/storage/builder.rs (1)
112-116:⚠️ Potential issue | 🟡 MinorUpdate the inline layer-order docs to match actual runtime layering.
Line 112 still documents
[FoyerCache] → OtelMetrics → OtelTrace → S3, but Lines 135-139 can addPrefetchLayeras outermost. Also, Line 133 implies cache is always present, while Line 125 makes it optional. Please make this comment block canonical and conditional.Suggested doc-only fix
- // [FoyerCache] → OtelMetrics → OtelTrace → S3 + // [Prefetch] → [FoyerCache] → OtelMetrics → OtelTrace → S3 @@ - // Prefetch layer sits outermost so background reads flow through the - // cache layer and land in the foyer cache. + // Prefetch layer sits outermost. When cache is enabled, background reads + // flow through the cache layer and warm it before foreground reads.Also applies to: 133-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/builder.rs` around lines 112 - 116, Update the comment block describing runtime layer order so it matches actual assembly and optionality: change the documented order from "[FoyerCache] → OtelMetrics → OtelTrace → S3" to include PrefetchLayer as the outermost when present (e.g. "PrefetchLayer → [FoyerCache] → OtelMetrics → OtelTrace → S3"), and make the cache notation conditional (bracketed) to reflect that FoyerCache may be absent; adjust the surrounding sentences to state that tracing/metrics sit below the cache (when present) so they only observe S3 round-trips, and ensure references to PrefetchLayer and FoyerCache in the comment block are consistent with actual runtime layering.crates/icegate-queue/src/writer.rs (1)
452-459:⚠️ Potential issue | 🟠 MajorLine 458 regresses the writer’s offset state after probing.
find_next_available_offset()stores the current candidate (offset) back into state. Withnext_offset(), this causes the next write cycle to re-probe/reuse the same slot pattern instead of advancing cleanly, which adds avoidable HEAD churn and can amplify conflict retries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 452 - 459, find_next_available_offset currently persists the probed candidate offset via set_offset(topic, offset) which regresses the writer state and causes repeated re-probing; change the behavior so probing does not clobber the writer's next-offset state — either remove the set_offset call entirely or persist offset + 1 instead (so update set_offset(topic, offset + 1)) and keep the function returning the first-free offset; update any callers (e.g., next_offset) expectations if you choose to advance by +1 and ensure SegmentId::new and segment_exists usage remains unchanged.crates/icegate-common/src/storage/icegate_storage.rs (1)
108-117:⚠️ Potential issue | 🟠 MajorAccept
s3://bucketas a valid root path.This branch only accepts
"{scheme}://{bucket}/". The exact bucket-root forms3://bucketis valid and should map to an empty relative path instead of failing validation.Possible fix
- let prefix = format!("{configured_scheme}://{}/", op_info.name()); - - let relative = if path.starts_with(&prefix) { - &path[prefix.len()..] - } else { + let base_prefix = format!("{configured_scheme}://{}", op_info.name()); + let with_slash = format!("{base_prefix}/"); + + let relative = if path == base_prefix { + "" + } else if let Some(rest) = path.strip_prefix(&with_slash) { + rest + } else { return Err(Error::new( ErrorKind::DataInvalid, - format!("Invalid S3 URL: {path}, expected prefix {prefix}"), + format!( + "Invalid S3 URL: {path}, expected prefix {base_prefix} or {with_slash}" + ), )); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 108 - 117, The code currently only accepts a bucket root with a trailing slash; update the logic that computes relative from path (using configured_scheme and op_info.name()) so it treats both "{scheme}://{bucket}/" and the exact "{scheme}://{bucket}" as valid roots: if path starts_with the prefix including the trailing slash set relative to the slice after prefix, else if path == prefix_without_trailing_slash set relative to empty string, otherwise return the existing Error::new DataInvalid with the same message; adjust the variables prefix (configured_scheme and op_info.name()), relative, and the error branch accordingly.crates/icegate-query/src/engine/core.rs (1)
136-189:⚠️ Potential issue | 🟠 MajorMake
start_background_refresh()idempotent.This still spawns a new long-lived refresh task on every call. If startup wiring, tests, or helpers invoke it twice, the engine will rebuild and publish the provider twice every
refresh_interval.Possible fix
+use std::sync::atomic::{AtomicBool, Ordering}; + pub struct QueryEngine { @@ /// Interval between background refresh cycles. refresh_interval: Duration, + /// Ensures background refresh is only started once. + background_refresh_started: AtomicBool, } @@ rebuild_lock: tokio::sync::Mutex::new(()), max_age: DEFAULT_MAX_AGE, refresh_interval: DEFAULT_REFRESH_INTERVAL, + background_refresh_started: AtomicBool::new(false), } } @@ pub fn start_background_refresh(self: &Arc<Self>) { + if self + .background_refresh_started + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { + tracing::debug!("Background refresh already started; ignoring duplicate call"); + return; + } + let weak = Arc::downgrade(self); tokio::spawn(async move { // existing task body... }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/core.rs` around lines 136 - 189, start_background_refresh currently spawns a new tokio task every time it is called; make it idempotent by adding and checking a guard on the Engine (e.g. an AtomicBool named background_refresh_started or a Mutex<Option<JoinHandle<()>>> like background_task_handle) inside start_background_refresh: perform an atomic compare-and-set (or check-and-set under the mutex) at the top of start_background_refresh and return immediately if the guard indicates a task is already running, otherwise set the guard and then spawn the tokio task as currently implemented; ensure the guard remains set while the background loop runs (or store the JoinHandle) so subsequent calls don’t spawn duplicates and clear the guard only if you intentionally stop the task.
🧹 Nitpick comments (2)
config/kustomize/overlays/skaffold/values-icegate.yaml (2)
115-119: Validate local cluster capacity against new ingest requests.With Lines 115-119 and
ingest.replicaCount: 2, ingest alone requests ~5Gi memory; combined with query requests, this can exceed common local skaffold cluster allocations and cause Pending pods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/skaffold/values-icegate.yaml` around lines 115 - 119, The local Skaffold overlay resource requests for the ingest container (cpu/memory and limits keys) are too large for ingest.replicaCount: 2 and will likely exhaust a typical local cluster; reduce per-replica memory or replicaCount, or document required local cluster capacity. Update the ingest-related keys (memory under requests and limits and cpu as needed) to lower values (e.g., reduce memory from 2.5Gi to a value compatible with local testing) or change ingest.replicaCount to 1 for local overlay, and add a comment or README note indicating the minimum local cluster memory required when using ingest.replicaCount >1 so Pending pods are avoided.
66-66: Consider dialing back always-on module-level debug logs in skaffold.Lines 66 and 112 enable broad debug scopes (
opendal,datafusion_tracing, multiple icegate modules). This is useful for troubleshooting, but it can skew local perf/caching observations and generate heavy log I/O.Also applies to: 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/overlays/skaffold/values-icegate.yaml` at line 66, The rustLog entry currently enables always-on module-level debug for opendal, datafusion_tracing, icegate_query, icegate_common (and others); change the rustLog value to remove or lower these module-level debug scopes (e.g., set them to info or remove them) so only minimal/global logging remains in the skaffold overlay, or implement conditional enabling (via an environment-specific value or feature flag) so modules like opendal, datafusion_tracing, icegate_query, and icegate_common are not debug by default; update the rustLog key in this values-icegate.yaml 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/templates/configmap-ingest.yaml`:
- Around line 18-19: The template currently uses "with" which treats 0 as empty
and drops keys; update the stat_ttl_secs and max_prefetch_bytes blocks to
explicitly check presence instead of truthiness — for example replace the with
.Values.ingest.cache.statTtlSecs and with .Values.ingest.cache.maxPrefetchBytes
usage with presence checks like hasKey on .Values.ingest.cache (or an explicit
nil check) so that keys stat_ttl_secs and max_prefetch_bytes are rendered even
when the value is 0; adjust the template blocks that emit stat_ttl_secs and
max_prefetch_bytes accordingly to reference .Values.ingest.cache.statTtlSecs and
.Values.ingest.cache.maxPrefetchBytes inside the new conditional.
In `@config/kustomize/overlays/skaffold/values-icegate.yaml`:
- Around line 86-87: The skaffold overlay currently sets
ingest.prefetch.enabled: false which conflicts with the aws-glue overlay that
enables prefetch; verify whether disabling prefetch in the skaffold profile is
intentional for local/dev testing or accidental, and if GH-91 requires prefetch
coverage, change the skaffold overlay's ingest.prefetch.enabled to true (or
document why it remains false) so the ingest.prefetch code path can be
exercised; check the values-icegate.yaml entry for the prefetch setting and
update it or add a comment explaining the intentional difference.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 43-67: The S3 variant of the IceGateStorage enum serializes
sensitive credentials because its config: Arc<S3Config> is included in derived
Serialize/Deserialize; update the S3 variant to add #[serde(skip)] on the config
field (i.e., on the config: Arc<S3Config> in IceGateStorage::S3) so the S3Config
(and contained
access_key_id/secret_access_key/session_token/server_side_encryption_customer_key)
are omitted during serde serialization; apply the same #[serde(skip)] change to
the other matching S3 config occurrences mentioned (lines 154-156 equivalent) to
prevent credential leakage.
---
Duplicate comments:
In `@crates/icegate-common/src/storage/builder.rs`:
- Around line 112-116: Update the comment block describing runtime layer order
so it matches actual assembly and optionality: change the documented order from
"[FoyerCache] → OtelMetrics → OtelTrace → S3" to include PrefetchLayer as the
outermost when present (e.g. "PrefetchLayer → [FoyerCache] → OtelMetrics →
OtelTrace → S3"), and make the cache notation conditional (bracketed) to reflect
that FoyerCache may be absent; adjust the surrounding sentences to state that
tracing/metrics sit below the cache (when present) so they only observe S3
round-trips, and ensure references to PrefetchLayer and FoyerCache in the
comment block are consistent with actual runtime layering.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 108-117: The code currently only accepts a bucket root with a
trailing slash; update the logic that computes relative from path (using
configured_scheme and op_info.name()) so it treats both "{scheme}://{bucket}/"
and the exact "{scheme}://{bucket}" as valid roots: if path starts_with the
prefix including the trailing slash set relative to the slice after prefix, else
if path == prefix_without_trailing_slash set relative to empty string, otherwise
return the existing Error::new DataInvalid with the same message; adjust the
variables prefix (configured_scheme and op_info.name()), relative, and the error
branch accordingly.
In `@crates/icegate-query/src/engine/core.rs`:
- Around line 136-189: start_background_refresh currently spawns a new tokio
task every time it is called; make it idempotent by adding and checking a guard
on the Engine (e.g. an AtomicBool named background_refresh_started or a
Mutex<Option<JoinHandle<()>>> like background_task_handle) inside
start_background_refresh: perform an atomic compare-and-set (or check-and-set
under the mutex) at the top of start_background_refresh and return immediately
if the guard indicates a task is already running, otherwise set the guard and
then spawn the tokio task as currently implemented; ensure the guard remains set
while the background loop runs (or store the JoinHandle) so subsequent calls
don’t spawn duplicates and clear the guard only if you intentionally stop the
task.
In `@crates/icegate-queue/src/writer.rs`:
- Around line 452-459: find_next_available_offset currently persists the probed
candidate offset via set_offset(topic, offset) which regresses the writer state
and causes repeated re-probing; change the behavior so probing does not clobber
the writer's next-offset state — either remove the set_offset call entirely or
persist offset + 1 instead (so update set_offset(topic, offset + 1)) and keep
the function returning the first-free offset; update any callers (e.g.,
next_offset) expectations if you choose to advance by +1 and ensure
SegmentId::new and segment_exists usage remains unchanged.
---
Nitpick comments:
In `@config/kustomize/overlays/skaffold/values-icegate.yaml`:
- Around line 115-119: The local Skaffold overlay resource requests for the
ingest container (cpu/memory and limits keys) are too large for
ingest.replicaCount: 2 and will likely exhaust a typical local cluster; reduce
per-replica memory or replicaCount, or document required local cluster capacity.
Update the ingest-related keys (memory under requests and limits and cpu as
needed) to lower values (e.g., reduce memory from 2.5Gi to a value compatible
with local testing) or change ingest.replicaCount to 1 for local overlay, and
add a comment or README note indicating the minimum local cluster memory
required when using ingest.replicaCount >1 so Pending pods are avoided.
- Line 66: The rustLog entry currently enables always-on module-level debug for
opendal, datafusion_tracing, icegate_query, icegate_common (and others); change
the rustLog value to remove or lower these module-level debug scopes (e.g., set
them to info or remove them) so only minimal/global logging remains in the
skaffold overlay, or implement conditional enabling (via an environment-specific
value or feature flag) so modules like opendal, datafusion_tracing,
icegate_query, and icegate_common are not debug by default; update the rustLog
key in this values-icegate.yaml accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0469c63-92d9-4c31-ae8e-6caf28bae50f
📒 Files selected for processing (11)
config/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/values-jaeger.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-ingest/src/shift/parquet_meta_reader.rscrates/icegate-query/src/engine/core.rscrates/icegate-queue/src/writer.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- config/helm/icegate/values.yaml
…nd metric configurations - Deleted Helm templates for PVC and storage class: `pvc-cache.yaml` and `storageclass-cache.yaml`. - Integrated cache metrics registration in `foyer` for Prometheus scraping. - Improved default cache configurations, including `max_write_cache_size` and catalog refresh settings. - Adjusted Helm and Kustomize resources for optimized resource limits and caching. - Migrated from pinned `iceberg` and `opendal` revisions to stable versions (`v0.9.0` and `v0.55`). - Streamlined I/O and queue logic by removing unnecessary group-by logic and concatenation in WAL writes.
There was a problem hiding this comment.
Actionable comments posted: 7
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/Icegate-Ingest-1769530183590.json (1)
4334-4369:⚠️ Potential issue | 🟡 MinorThe unit and title are inconsistent with the query's per-second rate calculation.
The query
rate(icegate_ingest_shift_bytes_written_bytes_total[1m])returns bytes per second, but the panel is configured withunit: "bytes"and titled "Shift task: parquet write bytes [1m]". This mislabels throughput as a cumulative byte count. Other panels in the dashboard correctly use rate units like"reqps"and"cps"for similar rate-based queries on _total metrics.Either use
increase(icegate_ingest_shift_bytes_written_bytes_total[1m])to get cumulative bytes and keep the current unit and title, or update the unit to reflect throughput (e.g.,"Bps") and retitle to indicate per-second rates.Suggested fix (if keeping rate)
Change unit from
"bytes"to"Bps"and update title to indicate throughput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json` around lines 4334 - 4369, The panel with id 13 uses the Prometheus expression sum by (topic) (rate(icegate_ingest_shift_bytes_written_bytes_total{job="icegate-ingest"}[1m])) but is labeled and unit'ed as a cumulative byte count; fix this by either switching the query to increase(icegate_ingest_shift_bytes_written_bytes_total[1m]) if you want cumulative bytes, or (recommended) update the panel's unit from "bytes" to a throughput unit such as "Bps" and change the title "Shift task: parquet write bytes [1m]" to indicate rate (e.g., "Shift task: parquet write throughput [1m]" or "Shift task: parquet write Bps [1m]") so the unit, title, and the expr (rate(...)[1m]) are consistent.
♻️ Duplicate comments (3)
crates/icegate-common/src/storage/icegate_storage.rs (2)
133-141:⚠️ Potential issue | 🟠 MajorAccept exact bucket-root S3 URLs.
Lines 135-141 still reject
s3://bucket/s3a://bucket, even though that should map to an empty relative path. Root-level operations against a valid bucket URL will fail here.Suggested fix
- let prefix = format!("{configured_scheme}://{}/", op_info.name()); - - let relative = if path.starts_with(&prefix) { - &path[prefix.len()..] - } else { + let base_prefix = format!("{configured_scheme}://{}", op_info.name()); + let prefix = format!("{base_prefix}/"); + + let relative = if path == base_prefix { + "" + } else if let Some(rest) = path.strip_prefix(&prefix) { + rest + } else { return Err(Error::new( ErrorKind::DataInvalid, - format!("Invalid S3 URL: {path}, expected prefix {prefix}"), + format!("Invalid S3 URL: {path}, expected prefix {base_prefix} or {prefix}"), )); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 133 - 141, The current S3 URL handling rejects bucket-root URLs like "s3://bucket" because it only accepts prefixes with a trailing slash; update the check in the block using configured_scheme and op_info.name() so it also accepts the exact bucket root (no trailing slash) and maps it to an empty relative path. Concretely, after building prefix = format!("{configured_scheme}://{}/", op_info.name()), add logic to treat path == format!("{configured_scheme}://{}", op_info.name()) as relative = "" (or accept starts_with the prefix as before), so the variable relative is set to an empty string for bucket-root URLs instead of returning Error::new(ErrorKind::DataInvalid, ...).
57-61:⚠️ Potential issue | 🔴 CriticalAvoid serializing live
S3Configcredentials.
IceGateStoragederives serde, and Line 61 is the only S3 field that still serializes.opendal::services::S3Configincludes access keys and session tokens in its serde representation, so dumping or typetag-serializing this enum can leak credentials. Please use a redacted/custom representation here, or stop serializing the live storage object entirely.In the OpenDAL version used by this repo, does `opendal::services::S3Config` redact or skip fields such as `access_key_id`, `secret_access_key`, `session_token`, or `server_side_encryption_customer_key` during serde serialization?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/icegate_storage.rs` around lines 57 - 61, IceGateStorage's S3 variant currently holds config: Arc<S3Config> which will be serialized and can leak credentials; update the S3 variant to avoid serializing live S3Config by either (A) replacing config: Arc<S3Config> with a non-serializable field (e.g., #[serde(skip)] private s3_client: Arc<S3Config>) and add a separate serializable S3ConfigRedacted struct containing only safe metadata (e.g., configured_scheme, endpoint, bucket) used for serde, or (B) implement custom Serialize/Deserialize for IceGateStorage (or the S3 variant) that omits or redacts sensitive fields from opendal::services::S3Config; locate the enum IceGateStorage and the S3 variant and change the field name/config type accordingly and ensure any construction/use sites convert between S3Config and the redacted representation so runtime code still has access to credentials while serialized forms never include them.crates/icegate-query/src/engine/core.rs (1)
130-183:⚠️ Potential issue | 🟠 MajorPrevent duplicate provider rebuilds in the refresh path.
start_background_refresh()still spawns unconditionally, and Lines 136/164 callbuild_provider()outsiderebuild_lock. A secondstart_background_refresh()call—or the first query arriving while the eager refresh is still running—can still fan out duplicate catalog rebuilds. Make startup idempotent and take the same lock before each background build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/src/engine/core.rs` around lines 130 - 183, Make start_background_refresh idempotent and ensure builds use the same rebuild_lock: before spawning, return early if a background refresh is already running (e.g., set/check a boolean/Option<JoinHandle> on the Engine) so duplicate start_background_refresh() calls don't spawn a second task; and inside the spawned task, take engine.rebuild_lock (e.g., engine.rebuild_lock.lock().await) before calling Self::build_provider(Arc::clone(&engine.catalog), Arc::clone(&engine.wal_reader)) both for the eager initial build and for the periodic rebuild in the loop so concurrent callers/queries cannot fan out duplicate rebuilds, then release the lock after the provider is constructed and sent via provider_tx (CachedProvider).
🧹 Nitpick comments (6)
crates/icegate-queue/src/writer.rs (2)
369-393: Minor: Offset may be skipped on non-conflict write failures.The
claim_offsetmethod callsset_offset(topic, next)at line 387 before the actual PUT operation completes. Iftry_writesubsequently fails with an error other thanAlreadyExists, the counter has already advanced past the claimed offset.On the next
claim_offsetcall, probing starts from the advanced counter, potentially leaving a "hole" at the previously claimed offset. While this isn't a correctness issue (another replica can claim that offset, or the offset will remain unused), it may lead to non-contiguous offsets after transient failures.This is acceptable given the design prioritizes conflict resolution over gap-free offsets, but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 369 - 393, claim_offset advances the stored counter via self.set_offset(topic, next) before the actual write completes, which can leave holes if try_write fails with non-AlreadyExists errors; change the flow so the counter is only advanced after a successful PUT or when a conflict (AlreadyExists) is observed: move or defer the call to set_offset(topic, next) until after the successful write path in the write routine that uses claim_offset, or add logic to roll back/retry (call set_offset only on success or on AlreadyExists detection) — look for usages around claim_offset, set_offset, and try_write to implement this behavior.
479-507: Minor: Duplicate logging forAlreadyExistscondition.Lines 480-485 and line 495 both log debug messages when an
AlreadyExistserror occurs. Consider consolidating into a single log statement.♻️ Suggested consolidation
if matches!(e, object_store::Error::AlreadyExists { .. }) { debug!( reason = object_store_error_reason(&e), elapsed_ms, - error = %e, - "store.put_opts conflict (expected in multi-replica)" + offset = segment_id.offset, + "Segment already exists (expected in multi-replica), will probe for next available offset" ); } else { warn!( reason = object_store_error_reason(&e), elapsed_ms, error = %e, "store.put_opts failed" ); } if matches!(e, object_store::Error::AlreadyExists { .. }) { - debug!("Segment already exists, skipping write (offset {})", segment_id.offset); QueueError::AlreadyExists { topic: segment_id.topic.clone(), offset: segment_id.offset, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-queue/src/writer.rs` around lines 479 - 507, The code currently logs the AlreadyExists case twice: once in the first debug block within the matches!(e, object_store::Error::AlreadyExists { .. }) branch and again just before returning QueueError::AlreadyExists in the closure inside write handling in writer.rs; consolidate by removing the second debug call and keeping a single, informative log where object_store::Error::AlreadyExists is detected (use object_store_error_reason(&e), elapsed_ms, and error = %e), then return QueueError::AlreadyExists { topic: segment_id.topic.clone(), offset: segment_id.offset } without re-logging; ensure references to object_store::Error::AlreadyExists, object_store_error_reason, segment_id, and QueueError are used to find and update the code.config/kustomize/base/dashboards/Nodes-1769530183590.json (1)
2441-2459: Make the memory panels use one label dimension consistently.These queries now select on
name, but the same panels still render/aggregate onpod({{pod}}andby (pod)). Ifpodis really the label that disappeared, the legends will blank out and the RSS panel will collapse unrelated containers into one series. Please verify which label is actually present and align the selector, aggregation, and legend to that same label.Suggested dashboard diff
- "legendFormat": "{{pod}}", + "legendFormat": "{{name}}", - "legendFormat": "Limit {{pod}}", + "legendFormat": "Limit {{name}}", - "expr": "sum(container_memory_rss{name!=\"\"}) by (pod)", + "expr": "sum(container_memory_rss{name!=\"\"}) by (name)", - "legendFormat": "{{pod}}", + "legendFormat": "{{name}}",Also applies to: 2552-2558
🤖 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 memory panels use selectors like container_memory_usage_bytes{name!=""} and container_spec_memory_limit_bytes{name!=""} but still render/aggregate using the pod label (legendFormat "{{pod}}" and any "by (pod)" aggregations); pick the single correct label that exists (either "name" or "pod"), update the PromQL selectors to filter by that label, update any aggregation clauses (e.g., "by (pod)") to use the same label, and change the legendFormat (e.g., "{{name}}" or "{{pod}}") so selector, aggregation and legend all use the identical label (apply the same change to the other panel block around lines with legendFormat "{{pod}}" and the block with hide:true).crates/icegate-ingest/src/main.rs (1)
14-21: Remove unnecessary.to_string()allocation.
thread_nameacceptsimpl Into<String>, and a string literal&'static strimplementsInto<String>directly. The explicit.to_string()creates an unnecessary allocation.♻️ Proposed fix
let mut builder = Builder::new_multi_thread(); builder .worker_threads(plan.main_threads) - .thread_name("icegate-ingest-main".to_string()) + .thread_name("icegate-ingest-main") .enable_all();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-ingest/src/main.rs` around lines 14 - 21, The call to builder.thread_name currently uses an unnecessary allocation via ".to_string()"; change the invocation in the Builder::new_multi_thread setup to pass the string literal directly (builder.thread_name("icegate-ingest-main")) so the &str can be converted via Into<String> without the extra allocation—this affects the builder configuration around Builder::new_multi_thread and the builder.thread_name call before building the runtime.crates/icegate-common/src/catalog/config.rs (1)
156-170: Consider validatingmax_write_cache_size_mbwhen set.
stat_ttl_secsis validated to be greater than 0 when set, butmax_write_cache_size_mbhas no such validation. A value ofSome(0)would effectively disable write caching entirely — if this is intentional, consider documenting it; otherwise, add a similar validation check.♻️ Proposed validation
if cache.stat_ttl_secs == Some(0) { return Err(CommonError::Config( "Cache stat_ttl_secs must be greater than 0 when set".into(), )); } + if cache.max_write_cache_size_mb == Some(0) { + return Err(CommonError::Config( + "Cache max_write_cache_size_mb must be greater than 0 when set".into(), + )); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/catalog/config.rs` around lines 156 - 170, Add a validation for cache.max_write_cache_size_mb analogous to the existing stat_ttl_secs check: when validating the cache (the same block that inspects cache.stat_ttl_secs), ensure if cache.max_write_cache_size_mb == Some(0) you return Err(CommonError::Config("Cache max_write_cache_size_mb must be greater than 0 when set".into())); place this check alongside the stat_ttl_secs validation so max_write_cache_size_mb cannot be set to 0 unless you intentionally allow it.crates/icegate-common/src/storage/cache.rs (1)
683-690: Redundant cache lookup after lock acquisition.After acquiring the per-key lock, the code calls
cache.get(&key)again (line 685-690) even though it already performed a coverage check at lines 661-681. The second fetch clones theCacheValuefor mutation, but if the entry already covers the range (checked at 668), we return early. Consider combining these lookups to avoid the redundant 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 683 - 690, The per-key lock branch redundantly calls inner.cache.get(&key) and clones a CacheValue after you already inspected the cache for coverage earlier; instead reuse the previously obtained cache entry/value to avoid a second lookup and clone. Locate the earlier coverage check that inspects inner.cache.get(&key) and keep the retrieved entry or its cloned CacheValue (the variable produced during that check) across the lock acquisition (or move the clone before locking) so the code can mutate/merge fetched data without calling inner.cache.get(&key) again; update the logic around the per-key lock acquisition, the existing variable that holds the cache entry/value, and any early-return that checks coverage (using CacheValue) to rely on the reused value.
🤖 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/Icegate-Ingest-1769530183590.json`:
- Around line 3729-3735: The panel titled "Plan task: planned segments per shift
task [1m]" (refId "A") currently divides by
rate(icegate_ingest_shift_planned_tasks_total{job="icegate-ingest"}[1m]) which
can be zero; update the PromQL denominator to
clamp_min(rate(icegate_ingest_shift_planned_tasks_total{job="icegate-ingest"}[1m]),
1e-12) so the full expression becomes sum by
(topic)(rate(icegate_ingest_shift_planned_segments_total{job="icegate-ingest"}[1m]))
/ clamp_min(sum by
(topic)(rate(icegate_ingest_shift_planned_tasks_total{job="icegate-ingest"}[1m])),
1e-12) matching the guard pattern used by other ratio panels.
- Around line 4677-4686: The Prometheus expression in the panel titled "Commit
task: already commit count [1m]" uses rate() but should use increase() for
consistency with other count panels; update the expr value currently containing
sum by (topic)
(rate(icegate_ingest_shift_already_committed_total{job="icegate-ingest"}[1m]))
to use increase(...) instead (preserving the sum by (topic) and label filter) so
the panel reflects total count over the 1m range rather than a per-second rate.
In `@crates/icegate-common/src/catalog/builder.rs`:
- Around line 34-36: Doc example uses the old CatalogBuilder::from_config
signature with one argument; update it to pass the new prefetch_config argument
as required. Modify the snippet so the call to
CatalogBuilder::from_config(&config.catalog, &io_cache).await? becomes
CatalogBuilder::from_config(&config.catalog, &config.catalog.prefetch_config,
&io_cache).await? (or pass the appropriate prefetch_config value), and ensure
IoHandle::from_config usage remains consistent with its signature.
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 897-906: The current CacheDeleter::flush evicts keys
(self.inner.cache.remove, self.inner.stat_cache.remove, self.inner.locks.remove
and self.inner.metrics.record_eviction) before calling
self.deleter.flush().await, which can leave the cache inconsistent if the
backend flush fails; change the logic to call let result =
self.deleter.flush().await first, return the error immediately if it fails, and
only on success iterate over self.keys to perform tracing::trace!(...), remove
entries and record metrics, then return the successful result so eviction
happens only after backend flush succeeds.
- Around line 254-261: The function u64_to_usize currently does a direct cast
which truncates on 32-bit targets; change it to perform a saturating conversion
by checking if v > usize::MAX as u64 and returning usize::MAX in that case,
otherwise returning v as usize (preserving the const fn if possible); update or
remove the clippy allow attribute as needed and ensure the helper comment
reflects the actual saturating behavior.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 109-113: The code uses string strip_prefix on `root` which can
mis-handle paths (e.g., `/tmp/warehouse-backup/...` matching `/tmp/warehouse`);
instead convert the post-scheme path to a std::path::Path and use
Path::strip_prefix to enforce path-component boundaries. In the block that
computes `absolute` and `relative` (variables `path`, `absolute`, `relative`,
and `root` in icegate_storage.rs), replace the string-based
`strip_prefix`/`trim_*` logic with
Path::new(absolute).strip_prefix(Path::new(root_trimmed)) and, if strip_prefix
fails, treat the path as out-of-root and return an error (or otherwise
reject/skip) rather than falling back to the raw `absolute` string; this ensures
correct boundary checking using path components.
In `@crates/icegate-ingest/src/shift/parquet_meta_reader.rs`:
- Around line 330-349: The current loop over row-group column metadata
accumulates null_value_counts, lower_bounds, and upper_bounds even when some row
groups lack statistics or have non-exact/unconvertible min/max, producing
incorrect file-level "exact" bounds; fix it by tracking incomplete field IDs
(e.g., a HashSet like incomplete_fields) and, for each fid in the loop, if
col_meta.statistics() is None OR stats.null_count_opt() is None OR
stats.min_is_exact()/max_is_exact() is false OR
parquet_stat_min_as_datum/parquet_stat_max_as_datum returns Err/Ok(None), then
mark fid as incomplete and remove any existing entries from null_value_counts,
lower_bounds, and upper_bounds (and skip further updates for that fid);
otherwise continue using schema.field_by_id, Type::Primitive,
update_lower_bound, and update_upper_bound as before to accumulate only
fully-provided, exact values.
---
Outside diff comments:
In `@config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json`:
- Around line 4334-4369: The panel with id 13 uses the Prometheus expression sum
by (topic)
(rate(icegate_ingest_shift_bytes_written_bytes_total{job="icegate-ingest"}[1m]))
but is labeled and unit'ed as a cumulative byte count; fix this by either
switching the query to
increase(icegate_ingest_shift_bytes_written_bytes_total[1m]) if you want
cumulative bytes, or (recommended) update the panel's unit from "bytes" to a
throughput unit such as "Bps" and change the title "Shift task: parquet write
bytes [1m]" to indicate rate (e.g., "Shift task: parquet write throughput [1m]"
or "Shift task: parquet write Bps [1m]") so the unit, title, and the expr
(rate(...)[1m]) are consistent.
---
Duplicate comments:
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 133-141: The current S3 URL handling rejects bucket-root URLs like
"s3://bucket" because it only accepts prefixes with a trailing slash; update the
check in the block using configured_scheme and op_info.name() so it also accepts
the exact bucket root (no trailing slash) and maps it to an empty relative path.
Concretely, after building prefix = format!("{configured_scheme}://{}/",
op_info.name()), add logic to treat path == format!("{configured_scheme}://{}",
op_info.name()) as relative = "" (or accept starts_with the prefix as before),
so the variable relative is set to an empty string for bucket-root URLs instead
of returning Error::new(ErrorKind::DataInvalid, ...).
- Around line 57-61: IceGateStorage's S3 variant currently holds config:
Arc<S3Config> which will be serialized and can leak credentials; update the S3
variant to avoid serializing live S3Config by either (A) replacing config:
Arc<S3Config> with a non-serializable field (e.g., #[serde(skip)] private
s3_client: Arc<S3Config>) and add a separate serializable S3ConfigRedacted
struct containing only safe metadata (e.g., configured_scheme, endpoint, bucket)
used for serde, or (B) implement custom Serialize/Deserialize for IceGateStorage
(or the S3 variant) that omits or redacts sensitive fields from
opendal::services::S3Config; locate the enum IceGateStorage and the S3 variant
and change the field name/config type accordingly and ensure any
construction/use sites convert between S3Config and the redacted representation
so runtime code still has access to credentials while serialized forms never
include them.
In `@crates/icegate-query/src/engine/core.rs`:
- Around line 130-183: Make start_background_refresh idempotent and ensure
builds use the same rebuild_lock: before spawning, return early if a background
refresh is already running (e.g., set/check a boolean/Option<JoinHandle> on the
Engine) so duplicate start_background_refresh() calls don't spawn a second task;
and inside the spawned task, take engine.rebuild_lock (e.g.,
engine.rebuild_lock.lock().await) before calling
Self::build_provider(Arc::clone(&engine.catalog),
Arc::clone(&engine.wal_reader)) both for the eager initial build and for the
periodic rebuild in the loop so concurrent callers/queries cannot fan out
duplicate rebuilds, then release the lock after the provider is constructed and
sent via provider_tx (CachedProvider).
---
Nitpick comments:
In `@config/kustomize/base/dashboards/Nodes-1769530183590.json`:
- Around line 2441-2459: The memory panels use selectors like
container_memory_usage_bytes{name!=""} and
container_spec_memory_limit_bytes{name!=""} but still render/aggregate using the
pod label (legendFormat "{{pod}}" and any "by (pod)" aggregations); pick the
single correct label that exists (either "name" or "pod"), update the PromQL
selectors to filter by that label, update any aggregation clauses (e.g., "by
(pod)") to use the same label, and change the legendFormat (e.g., "{{name}}" or
"{{pod}}") so selector, aggregation and legend all use the identical label
(apply the same change to the other panel block around lines with legendFormat
"{{pod}}" and the block with hide:true).
In `@crates/icegate-common/src/catalog/config.rs`:
- Around line 156-170: Add a validation for cache.max_write_cache_size_mb
analogous to the existing stat_ttl_secs check: when validating the cache (the
same block that inspects cache.stat_ttl_secs), ensure if
cache.max_write_cache_size_mb == Some(0) you return
Err(CommonError::Config("Cache max_write_cache_size_mb must be greater than 0
when set".into())); place this check alongside the stat_ttl_secs validation so
max_write_cache_size_mb cannot be set to 0 unless you intentionally allow it.
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 683-690: The per-key lock branch redundantly calls
inner.cache.get(&key) and clones a CacheValue after you already inspected the
cache for coverage earlier; instead reuse the previously obtained cache
entry/value to avoid a second lookup and clone. Locate the earlier coverage
check that inspects inner.cache.get(&key) and keep the retrieved entry or its
cloned CacheValue (the variable produced during that check) across the lock
acquisition (or move the clone before locking) so the code can mutate/merge
fetched data without calling inner.cache.get(&key) again; update the logic
around the per-key lock acquisition, the existing variable that holds the cache
entry/value, and any early-return that checks coverage (using CacheValue) to
rely on the reused value.
In `@crates/icegate-ingest/src/main.rs`:
- Around line 14-21: The call to builder.thread_name currently uses an
unnecessary allocation via ".to_string()"; change the invocation in the
Builder::new_multi_thread setup to pass the string literal directly
(builder.thread_name("icegate-ingest-main")) so the &str can be converted via
Into<String> without the extra allocation—this affects the builder configuration
around Builder::new_multi_thread and the builder.thread_name call before
building the runtime.
In `@crates/icegate-queue/src/writer.rs`:
- Around line 369-393: claim_offset advances the stored counter via
self.set_offset(topic, next) before the actual write completes, which can leave
holes if try_write fails with non-AlreadyExists errors; change the flow so the
counter is only advanced after a successful PUT or when a conflict
(AlreadyExists) is observed: move or defer the call to set_offset(topic, next)
until after the successful write path in the write routine that uses
claim_offset, or add logic to roll back/retry (call set_offset only on success
or on AlreadyExists detection) — look for usages around claim_offset,
set_offset, and try_write to implement this behavior.
- Around line 479-507: The code currently logs the AlreadyExists case twice:
once in the first debug block within the matches!(e,
object_store::Error::AlreadyExists { .. }) branch and again just before
returning QueueError::AlreadyExists in the closure inside write handling in
writer.rs; consolidate by removing the second debug call and keeping a single,
informative log where object_store::Error::AlreadyExists is detected (use
object_store_error_reason(&e), elapsed_ms, and error = %e), then return
QueueError::AlreadyExists { topic: segment_id.topic.clone(), offset:
segment_id.offset } without re-logging; ensure references to
object_store::Error::AlreadyExists, object_store_error_reason, segment_id, and
QueueError are used to find and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d29ff6c-6aad-4f40-9873-1935323e12e8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
Cargo.tomlconfig/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/base/dashboards/Icegate-Ingest-1769530183590.jsonconfig/kustomize/base/dashboards/Nodes-1769530183590.jsonconfig/kustomize/base/values-grafana.yamlconfig/kustomize/base/values-jaeger.yamlconfig/kustomize/base/values-prometheus.yamlconfig/kustomize/overlays/aws-glue/kustomization.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-minio.yamlconfig/kustomize/overlays/skaffold/values-nessie.yamlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-common/src/lib.rscrates/icegate-common/src/metrics.rscrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_s3.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-common/src/storage/mod.rscrates/icegate-common/src/tracing.rscrates/icegate-ingest/Cargo.tomlcrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-ingest/src/main.rscrates/icegate-ingest/src/otlp_grpc/services.rscrates/icegate-ingest/src/otlp_http/handlers.rscrates/icegate-ingest/src/shift/parquet_meta_reader.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/table.rscrates/icegate-queue/README.mdcrates/icegate-queue/benches/queue_s3_bench.rscrates/icegate-queue/src/accumulator.rscrates/icegate-queue/src/channel.rscrates/icegate-queue/src/lib.rscrates/icegate-queue/src/writer.rscrates/icegate-queue/tests/reader_integration.rscrates/icegate-queue/tests/recovery_integration.rscrates/icegate-queue/tests/writer_integration.rs
💤 Files with no reviewable changes (5)
- crates/icegate-ingest/src/otlp_http/handlers.rs
- crates/icegate-ingest/src/otlp_grpc/services.rs
- crates/icegate-queue/README.md
- crates/icegate-queue/src/lib.rs
- crates/icegate-queue/src/channel.rs
✅ Files skipped from review due to trivial changes (5)
- crates/icegate-common/src/tracing.rs
- config/kustomize/base/values-jaeger.yaml
- config/kustomize/overlays/aws-glue/kustomization.yaml
- config/helm/icegate/templates/configmap-query.yaml
- crates/icegate-queue/benches/queue_s3_bench.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- config/kustomize/base/values-grafana.yaml
- crates/icegate-ingest/Cargo.toml
- crates/icegate-query/src/engine/provider/table.rs
- config/helm/icegate/templates/configmap-ingest.yaml
- config/kustomize/base/values-prometheus.yaml
- config/kustomize/overlays/aws-glue/values-icegate.yaml
- crates/icegate-common/src/storage/mod.rs
- crates/icegate-query/src/cli/commands/run.rs
- crates/icegate-common/src/storage/icegate_s3.rs
- Cargo.toml
- crates/icegate-common/src/storage/builder.rs
…nd enhance cache configuration: - Added `AtomicBool` to ensure `start_background_refresh` is invoked only once. - Synchronized background refresh with a lock to prevent racing with synchronous builds. - Improved validation for `max_write_cache_size_mb` in cache configuration. - Optimized queue offset handling and error management for better reliability. - Updated Grafana dashboards for enhanced metrics visualization (e.g., clamped rates, corrected units, etc.). - Enhanced S3 storage implementation to prevent credential exposure in serialization and improve root path handling.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
crates/icegate-common/src/storage/builder.rs (1)
4-5:⚠️ Potential issue | 🟡 MinorPlease realign the layer-stack comments with the implementation.
The header still says this path includes retry, and the mid-function “desired order” block still omits the outer
PrefetchLayer, but the code here buildsPrefetch -> Cache -> OtelMetrics -> OtelTrace -> S3when all layers are enabled.Also applies to: 32-35, 114-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-common/src/storage/builder.rs` around lines 4 - 5, Update the top-of-file and in-function "desired order" comments in builder.rs to match the actual layer construction implemented by StorageBuilder::build (and related functions) which composes PrefetchLayer -> CacheLayer -> OtelMetricsLayer -> OtelTraceLayer -> S3 backend when all layers are enabled; specifically mention PrefetchLayer in the header and insert the full ordered list (Prefetch, Cache, OtelMetrics, OtelTrace, S3) where the "desired order" block is documented so the comments align with the implemented layering (references: PrefetchLayer, CacheLayer, OtelMetricsLayer, OtelTraceLayer, and the function that composes them in builder.rs).
🤖 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/Icegate-Ingest-1769530183590.json`:
- Around line 3317-3338: The legends omit status while the queries aggregate by
(topic, status), causing duplicate labels; update the panels that use
histogram_quantile over
icegate_ingest_shift_get_last_offset_duration_seconds_bucket (refId A/B/C) and
the similar panels over shift_commit_duration_seconds_bucket to either remove
status from the sum by(...) grouping or include status in the legendFormat (e.g.
change "{{topic}} - p95" / "{{topic}} - p50" / "{{topic}} - p99" to include
"{{status}}" or another distinguishing label) so each series is uniquely
identifiable; locate the queries referencing
icegate_ingest_shift_get_last_offset_duration_seconds_bucket and
shift_commit_duration_seconds_bucket and apply the chosen fix consistently.
- Line 4868: The numerator in the PromQL expression for the success-rate panel
only returns series when icegate_ingest_shift_task_success_total has samples,
causing tasks with only failures to disappear; update the numerator (the sum by
(topic, task)
(rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m]))) to
include an "or ... * 0" zero-fill pattern — e.g., append "or sum by (topic,
task)
(rate(icegate_ingest_shift_task_failures_total{job=\"icegate-ingest\"}[1m])) *
0" (or the success metric multiplied by 0) so the numerator produces a zero
series for (topic, task) pairs that only have failures, matching the
denominator's clamp_min behavior.
- Around line 1937-1942: The Tokio collapsed row was moved to y:31 but its child
panels still have y positions at 26, 32, 38 etc., causing overlap; locate the
collapsed row object with "collapsed": true and gridPos.y == 31 (the Tokio row)
and then add +6 to the gridPos.y of every child panel in that Tokio panels array
(search for panel objects whose titles/ids belong to the Tokio group or whose
gridPos.y values are 26, 32, 38, etc.) so their y values become 32, 38, 44
(apply the same +6 offset to any other Tokio child panel entries found in that
array).
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 821-851: The current write()/close() flow buffers every chunk into
self.buf and only checks max_write_cache_size in close(), causing
double-buffering and leaving stale cache entries when the limit is exceeded;
modify write() to track an incremental buffered byte counter (e.g.,
self.buffered_bytes) and, on each write, stop retaining further chunks into
self.buf once max_write_cache_size is exceeded (so large payloads aren’t
stored), and update close() so that if the cache is not repopulated (exceeds
limit) you still evict any previous entries by calling
self.inner.cache.remove(&key) and self.inner.stat_cache.remove(&key) after a
successful self.w.close().await; keep references to symbols: write, close,
self.buf, self.buffered_bytes (add if needed), self.inner.max_write_cache_size,
self.inner.cache.insert/remove, and self.inner.stat_cache.remove, and ensure
metrics.record_write_bytes(bytes_written) continues to be called.
- Around line 641-648: The full-file read path in read() computes range_end by
calling inner.accessor.stat(&path, ...) directly, bypassing the cached stat
lookup and defeating stat_ttl; refactor the stat-cache lookup into a shared
helper (e.g., a new async helper like cached_stat(&inner, &path) or reuse the
existing stat() wrapper) that returns the rp_stat/metadata, and call that helper
both where range_size is None (to compute range_end via
rp_stat.into_metadata().content_length()) and in the other affected section (the
same stat use around rp_stat/range_end later), so all full-file reads go through
the stat TTL cache instead of hitting inner.accessor.stat() directly.
In `@crates/icegate-common/src/storage/icegate_storage.rs`:
- Around line 172-195: The current create_operator() rebuilds the layered
operator (including CacheLayer/KeyLocks/stat_cache) on every call causing lost
stat_ttl and races; fix by making the bucket-specific operator or mutable cache
state persistent: either store a per-bucket layered operator cache on the
Storage struct (e.g., a HashMap<Bucket, Operator> guarded by Arc/RwLock) and
return cloned operators instead of rebuilding, or move the mutable pieces out of
CacheLayer into shared Arcs (e.g., create shared KeyLocks and stat_cache in
Storage and pass Arc clones into CacheLayer::new), updating create_operator() to
reuse those shared instances so stat_ttl and key-level state survive across
Storage method calls.
- Around line 150-162: The prefix comparison is too strict about
configured_scheme (e.g., "s3" vs "s3a") and rejects valid Iceberg locations;
update the logic around configured_scheme/prefix in the block that computes
relative so it treats "s3" and "s3a" as equivalent: compute both prefixes
(configured_scheme://{op_info.name()}/ and the alternate scheme variant by
swapping "s3"↔"s3a"), then check path against either prefix (and against either
trimmed bucket-root form) before returning DataInvalid; keep the same error path
if neither matches and reference configured_scheme, op_info.name(),
prefix/alternate_prefix and the relative variable when adjusting the logic.
---
Duplicate comments:
In `@crates/icegate-common/src/storage/builder.rs`:
- Around line 4-5: Update the top-of-file and in-function "desired order"
comments in builder.rs to match the actual layer construction implemented by
StorageBuilder::build (and related functions) which composes PrefetchLayer ->
CacheLayer -> OtelMetricsLayer -> OtelTraceLayer -> S3 backend when all layers
are enabled; specifically mention PrefetchLayer in the header and insert the
full ordered list (Prefetch, Cache, OtelMetrics, OtelTrace, S3) where the
"desired order" block is documented so the comments align with the implemented
layering (references: PrefetchLayer, CacheLayer, OtelMetricsLayer,
OtelTraceLayer, and the function that composes them in builder.rs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb11f008-4e53-40b4-a4c5-666f6d8eba01
📒 Files selected for processing (14)
config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.jsonconfig/kustomize/base/dashboards/Nodes-1769530183590.jsoncrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-ingest/src/main.rscrates/icegate-query/src/engine/core.rscrates/icegate-queue/benches/queue_s3_bench.rscrates/icegate-queue/src/writer.rscrates/icegate-queue/tests/reader_integration.rscrates/icegate-queue/tests/recovery_integration.rscrates/icegate-queue/tests/writer_integration.rs
✅ Files skipped from review due to trivial changes (4)
- crates/icegate-ingest/src/main.rs
- crates/icegate-queue/tests/recovery_integration.rs
- config/kustomize/base/dashboards/Nodes-1769530183590.json
- crates/icegate-common/src/catalog/config.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/icegate-queue/tests/writer_integration.rs
- crates/icegate-queue/benches/queue_s3_bench.rs
- crates/icegate-queue/tests/reader_integration.rs
- crates/icegate-queue/src/writer.rs
- crates/icegate-query/src/engine/core.rs
- crates/icegate-common/src/catalog/builder.rs
- Adjusted y-coordinates of widgets in Grafana dashboards for improved layout. - Simplified Prometheus expressions by removing redundant `status` label in histogram queries. - Improved Parquet metadata handling to account for incomplete statistics. - Enhanced S3 caching by switching to pre-built `CacheLayer` and adding `statTtlSecs` for metadata caching. - Updated Rust dependencies (`anstream`, `aws-sdk-s3`, etc.) and added new RUSTSEC exemption for `rustls-webpki`.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json (3)
4569-4593:⚠️ Potential issue | 🟡 MinorDisambiguate the per-status latency series.
These queries still aggregate by
(topic, status), but the legends only render{{topic}}, so different statuses collapse to identical labels in Grafana. Quick verify: filter to a topic that emits more than one status and the legend will show duplicatep50/p95/p99entries.Suggested legend fix
- "legendFormat": "{{topic}} - p95", + "legendFormat": "{{topic}} - {{status}} - p95", - "legendFormat": "{{topic}} - p50", + "legendFormat": "{{topic}} - {{status}} - p50", - "legendFormat": "{{topic}} - p99", + "legendFormat": "{{topic}} - {{status}} - p99",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json` around lines 4569 - 4593, The panel "Commit task: get parquet files latency [1m]" uses Prometheus expressions (e.g., icegate_ingest_shift_get_data_files_duration_seconds_bucket) that aggregate by (le, topic, status) but the legendFormat for series with refId A/B/C only shows {{topic}}, causing different statuses to collapse; update the legendFormat for the relevant queries (refId A, B, C) to include the status label (for example include {{status}} alongside {{topic}} and the percentile) so each series is disambiguated in Grafana.
4961-4967:⚠️ Potential issue | 🟡 MinorUse
increase()here or rename this as a rate panel.
rate(task_failures_total[1m])returns per-second values, but the neighboring success panel is a 1-minute count. That makes errors and successes non-comparable. Quick verify: a single failure in the last minute will render around0.0167, not1.Suggested count-based query
- "expr": "sum by (topic, task, reason) (rate(icegate_ingest_shift_task_failures_total{job=\"icegate-ingest\"}[1m]))\n", + "expr": "sum by (topic, task, reason) (increase(icegate_ingest_shift_task_failures_total{job=\"icegate-ingest\"}[1m]))\n",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json` around lines 4961 - 4967, The panel titled "Tasks: errors [1m]" is using rate(icegate_ingest_shift_task_failures_total[1m]) which produces per-second rates and is not comparable to the neighboring 1-minute count panel; change the query to use increase(icegate_ingest_shift_task_failures_total[1m]) (or alternatively rename the panel to indicate it's a per-second rate) so the metric (icegate_ingest_shift_task_failures_total) and refId "A" return a 1-minute count consistent with the success panel.
4868-4874:⚠️ Potential issue | 🟠 MajorZero-fill the first denominator branch too.
The numerator is padded now, but the denominator still starts with the raw success vector. For
(topic, task)pairs that only emit failures, the+drops the series beforeclamp_min()runs, so the panel still disappears instead of showing0. Quick verify: pick a task that had failures but no successes in the last minute; it will still be missing from this panel.Suggested PromQL fix
- ( - sum by (topic, task) (rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) - ) + + ( + sum by (topic, task) (rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) + or + sum by (topic, task) (rate(icegate_ingest_shift_task_failures_total{job="icegate-ingest"}[1m])) * 0 + ) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json` around lines 4868 - 4874, The denominator's first branch uses raw sum by (topic, task) (rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) which drops series that only have failures; mirror the numerator's zero-fill pattern by OR-ing a zeroed success vector into that first branch (i.e., change the first denominator term to sum by (topic, task) (rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) or sum by (topic, task) (rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) * 0) so clamp_min(...) receives the padded zero series and tasks with only failures show as 0 instead of disappearing; keep the rest (failures branch, clamp_min) unchanged.crates/icegate-common/src/storage/cache.rs (1)
236-240:⚠️ Potential issue | 🟠 MajorPath-only cache keys alias versioned and conditional operations.
CacheKeyonly storespath,cached_stat()replaces the caller's args withOpStat::default(), miss fills rebuildOpRead::default().with_range(...), and delete eviction ignoresOpDeleteoptions. OpenDAL 0.55 exposesversionand conditional fields on these ops, so different object versions or ETag-constrained reads can share or evict the same cache entry here. Encode those dimensions into the cache/stat keys, or bypass caching whenever the op args are non-default. (docs.rs)In OpenDAL 0.55, what version and conditional fields are available on `OpRead`, `OpStat`, and `OpDelete`, and should a caching layer preserve them?Also applies to: 470-470, 567-587, 627-638, 741-747, 777-783, 930-942
🤖 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 236 - 240, CacheKey currently only includes `path`, which conflates different object versions and conditional operations; update caching to incorporate OpenDAL op metadata or skip caching when ops are non-default: modify `CacheKey` to include relevant fields (e.g., `version`/`generation`, conditional/ETag fields) used by `cached_stat()`, the read miss path that rebuilds `OpRead::default().with_range(...)`, and the delete/eviction logic that currently ignores `OpDelete` options, or alternatively detect non-default `OpRead`, `OpStat`, and `OpDelete` args and bypass cache/stat storage/eviction; ensure `cached_stat()` preserves `OpStat`'s version/conditional values instead of replacing them with `OpStat::default()`.
🧹 Nitpick comments (1)
crates/icegate-ingest/src/shift/parquet_meta_reader.rs (1)
447-468: Consider adding Decimal type support.The conversion functions handle most common primitive types but omit
PrimitiveType::Decimal. If any tables use Decimal columns (common for financial data), their bounds will be marked incomplete, reducing pruning effectiveness.Decimal statistics in Parquet are stored as
Statistics::FixedLenByteArrayorStatistics::ByteArraydepending on precision. Adding support would require decoding the byte representation based on the decimal's precision and scale from the Iceberg type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-ingest/src/shift/parquet_meta_reader.rs` around lines 447 - 468, parquet_stat_min_as_datum currently omits PrimitiveType::Decimal; add a match arm for PrimitiveType::Decimal that handles Statistics::FixedLenByteArray and Statistics::ByteArray by extracting stats.min_opt(), decoding the parquet decimal bytes into the correct numeric value using the decimal precision/scale from the PrimitiveType::Decimal metadata, and returning Some(Datum::decimal(...)) (propagating errors as iceberg::Error like other arms); reference the function parquet_stat_min_as_datum, the Decimal variant PrimitiveType::Decimal, and the parquet stats variants Statistics::FixedLenByteArray and Statistics::ByteArray when locating where to add this logic and ensure error propagation matches existing uses of the ? operator.
🤖 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 1044-1084: The four observable instruments created for disk
metrics (the closures that call c.statistics().disk_write_bytes(),
disk_read_bytes(), disk_write_ios(), and disk_read_ios() after cloning cache)
should use u64_observable_counter instead of u64_observable_gauge; update each
meter.u64_observable_gauge(...) call to meter.u64_observable_counter(...),
preserving the description, unit, callback closure (including the c.clone move
and cast allow), and build() call so the metrics are registered as monotonic
counters for OpenTelemetry/Prometheus.
- Around line 603-608: make_read_response currently calls
BytesContentRange::with_size(range_end) which incorrectly uses the range end as
the total object size; change the signature of make_read_response to accept the
actual full object size (e.g., make_read_response(data: Vec<u8>, range_start:
u64, range_end: u64, total_size: u64)) and pass total_size to
BytesContentRange::with_size(total_size); update callers to supply the true
object size from the original response or cached stat metadata so Buffer::len()
or range_end are not used as the full size when building the RpRead response.
---
Duplicate comments:
In `@config/kustomize/base/dashboards/Icegate-Ingest-1769530183590.json`:
- Around line 4569-4593: The panel "Commit task: get parquet files latency [1m]"
uses Prometheus expressions (e.g.,
icegate_ingest_shift_get_data_files_duration_seconds_bucket) that aggregate by
(le, topic, status) but the legendFormat for series with refId A/B/C only shows
{{topic}}, causing different statuses to collapse; update the legendFormat for
the relevant queries (refId A, B, C) to include the status label (for example
include {{status}} alongside {{topic}} and the percentile) so each series is
disambiguated in Grafana.
- Around line 4961-4967: The panel titled "Tasks: errors [1m]" is using
rate(icegate_ingest_shift_task_failures_total[1m]) which produces per-second
rates and is not comparable to the neighboring 1-minute count panel; change the
query to use increase(icegate_ingest_shift_task_failures_total[1m]) (or
alternatively rename the panel to indicate it's a per-second rate) so the metric
(icegate_ingest_shift_task_failures_total) and refId "A" return a 1-minute count
consistent with the success panel.
- Around line 4868-4874: The denominator's first branch uses raw sum by (topic,
task) (rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m]))
which drops series that only have failures; mirror the numerator's zero-fill
pattern by OR-ing a zeroed success vector into that first branch (i.e., change
the first denominator term to sum by (topic, task)
(rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) or sum
by (topic, task)
(rate(icegate_ingest_shift_task_success_total{job="icegate-ingest"}[1m])) * 0)
so clamp_min(...) receives the padded zero series and tasks with only failures
show as 0 instead of disappearing; keep the rest (failures branch, clamp_min)
unchanged.
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 236-240: CacheKey currently only includes `path`, which conflates
different object versions and conditional operations; update caching to
incorporate OpenDAL op metadata or skip caching when ops are non-default: modify
`CacheKey` to include relevant fields (e.g., `version`/`generation`,
conditional/ETag fields) used by `cached_stat()`, the read miss path that
rebuilds `OpRead::default().with_range(...)`, and the delete/eviction logic that
currently ignores `OpDelete` options, or alternatively detect non-default
`OpRead`, `OpStat`, and `OpDelete` args and bypass cache/stat storage/eviction;
ensure `cached_stat()` preserves `OpStat`'s version/conditional values instead
of replacing them with `OpStat::default()`.
---
Nitpick comments:
In `@crates/icegate-ingest/src/shift/parquet_meta_reader.rs`:
- Around line 447-468: parquet_stat_min_as_datum currently omits
PrimitiveType::Decimal; add a match arm for PrimitiveType::Decimal that handles
Statistics::FixedLenByteArray and Statistics::ByteArray by extracting
stats.min_opt(), decoding the parquet decimal bytes into the correct numeric
value using the decimal precision/scale from the PrimitiveType::Decimal
metadata, and returning Some(Datum::decimal(...)) (propagating errors as
iceberg::Error like other arms); reference the function
parquet_stat_min_as_datum, the Decimal variant PrimitiveType::Decimal, and the
parquet stats variants Statistics::FixedLenByteArray and Statistics::ByteArray
when locating where to add this logic and ensure error propagation matches
existing uses of the ? operator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eccc6cfd-b131-497a-bf1a-f3c3c0446762
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.cargo/audit.tomlCargo.tomlconfig/kustomize/base/dashboards/Icegate-Ingest-1769530183590.jsoncrates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_storage.rscrates/icegate-ingest/src/shift/parquet_meta_reader.rs
✅ Files skipped from review due to trivial changes (1)
- .cargo/audit.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/icegate-common/src/storage/builder.rs
…registration: - Added `status` labels in Grafana `p95`, `p50`, and `p99` legends for improved clarity. - Optimized Prometheus expressions by replacing `rate` with `increase` for task failure queries. - Improved `Content-Range` handling in caching with support for `total_size` in read responses. - Switched observable gauges to counters for disk I/O metrics in Prometheus.
- Added background sweeper in `CacheLayer` with configurable intervals to prune idle locks and expired stat entries. - Refactored `StatCache` with dedicated pruning methods and wrapped `DashMap` for enhanced maintainability. - Improved memory management by integrating pruning logic into S3 storage initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/icegate-common/src/storage/cache.rs (1)
1034-1052: Consider clearingkeysafter successful flush.If
flush()is called multiple times (e.g., batch delete patterns), the same keys would be evicted repeatedly. While harmless, clearingself.keysafter the loop would be cleaner.🔧 Suggested fix
async fn flush(&mut self) -> Result<usize> { let count = self.deleter.flush().await?; for key in &self.keys { tracing::trace!(path = key.path, "Cache evicted on delete"); self.inner.cache.remove(key); self.inner.stat_cache.remove(key); self.inner.locks.remove(key); self.inner.metrics.record_eviction(); } + self.keys.clear(); Ok(count) }🤖 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 1034 - 1052, The flush implementation in CacheDeleter<A> evicts entries stored in self.keys but does not clear self.keys afterward, causing repeated evictions on subsequent flush calls; after successfully iterating and evicting keys in CacheDeleter::flush (and after confirming deleter.flush() succeeded and before returning), clear self.keys (e.g., self.keys.clear()) so the vector is reset for future operations while preserving existing behavior on error.
🤖 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/icegate_storage.rs`:
- Around line 209-226: The helper from_opendal_error currently maps all
opendal::Error kinds to iceberg::ErrorKind::Unexpected, which loses
opendal::ErrorKind::NotFound; update from_opendal_error (used by exists,
metadata, read in icegate_storage.rs) to match opendal::ErrorKind::NotFound and
return iceberg::ErrorKind::NotFound for that case, otherwise map to Unexpected,
and attach the original opendal::Error as the source so callers of metadata(),
read(), and exists() can distinguish missing-file errors from other I/O
failures.
---
Nitpick comments:
In `@crates/icegate-common/src/storage/cache.rs`:
- Around line 1034-1052: The flush implementation in CacheDeleter<A> evicts
entries stored in self.keys but does not clear self.keys afterward, causing
repeated evictions on subsequent flush calls; after successfully iterating and
evicting keys in CacheDeleter::flush (and after confirming deleter.flush()
succeeded and before returning), clear self.keys (e.g., self.keys.clear()) so
the vector is reset for future operations while preserving existing behavior on
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f968884a-e649-424c-b295-78f463f4aed5
📒 Files selected for processing (3)
crates/icegate-common/src/storage/builder.rscrates/icegate-common/src/storage/cache.rscrates/icegate-common/src/storage/icegate_storage.rs
✅ Files skipped from review due to trivial changes (1)
- crates/icegate-common/src/storage/builder.rs
- Simplified `IoHandle` initialization by removing `prefetch` argument. - Consolidated `prefetch` under `cache` in config structures for improved clarity. - Updated Helm and Kustomize templates to align with refactored `prefetch` structure. - Adjusted tests, examples, and code comments to reflect `prefetch` integration into `cache`. - Improved validation for `cache.prefetch` fields.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/overlays/aws-glue/values-icegate.yaml`:
- Around line 39-47: The overlay reserves too much cache memory (memorySizeMb:
1024) relative to the query pod memory (query.resources.memory left at 2Gi),
causing low headroom and OOM risk; fix by either reducing memorySizeMb to a
safer fraction (e.g., <= 512) or increasing the query pod memory request/limit
in the same overlay (adjust query.resources.memory), and ensure the chosen
change is applied consistently in values-icegate.yaml so DataFusion/Arrow
allocations have sufficient headroom.
In `@crates/icegate-common/src/catalog/builder.rs`:
- Around line 76-78: Summary: The MiB→bytes conversion for max_write_cache_size
currently uses raw multiplication and can overflow; replace it with a checked
multiplication to avoid wraparound. Fix: in the chain that computes
max_write_cache_size from cache_config and max_write_cache_size_mb, replace the
plain map(|mb| mb * 1024 * 1024) with a checked multiplication (e.g.,
mb.checked_mul(1024 * 1024)) and propagate the Option properly so overflow
yields None (or handle saturation if you prefer), ensuring types are adjusted
(cast to a wider integer like u64 first if needed); target identifiers:
max_write_cache_size, cache_config, max_write_cache_size_mb.
In `@crates/icegate-ingest/src/cli/commands/run.rs`:
- Around line 251-267: The io_cache (IoHandle) created by IoHandle::from_config
is not guaranteed to have close().await called on early returns from the rest of
the startup path (many `?` exits), risking the flusher race; modify the
post-cache startup code in run.rs so that once io_cache exists you scope the
remaining startup work in a block or use a drop-guard/finally-style await that
always calls io_cache.close().await on every exit (success or error).
Concretely, after creating io_cache call something like a scoped async block or
attach a guard that ensures IoHandle::close().await is invoked when leaving that
scope, and keep using
io_cache.cache()/prefetch()/stat_ttl()/max_write_cache_size() (and functions
like create_object_store and register_foyer_metrics) inside that protected scope
so any early `?` returns still trigger io_cache.close().await.
- Around line 289-299: The wal_runtime is created as a local Tokio Runtime in
execute() which will be dropped inside async context causing a panic; refactor
to follow the spawn_shift_runtime() pattern: create the WAL runtime on a
dedicated thread, use a shutdown channel and a wrapper struct (e.g.,
WalRuntimeHandle) that owns the thread JoinHandle, move the builder/enable_all
logic into the thread, send work (write_rx) to that thread and call writer.start
from inside that thread, store the thread handle in the wrapper, and ensure
execute() explicitly signals shutdown and joins the thread (calling the
wrapper.shutdown/join) before returning or on error so the Runtime is dropped on
the thread rather than inside the async function.
In `@crates/icegate-query/tests/loki/harness.rs`:
- Line 79: The harness builds a catalog with CatalogBuilder::from_config and
returns it via TestServer::start() but never kicks off the query engine's
background refresh, so later test appends can be missed; after you construct the
QueryEngine instance in this harness (the same place you call
CatalogBuilder::from_config and create the engine passed to TestServer), call
QueryEngine::start_background_refresh() (or spawn its background task) and
ensure it runs for the lifetime of the TestServer so the engine refreshes
provider snapshots when tests append data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57340bec-fda5-4f37-9b2b-b88e29a9f012
📒 Files selected for processing (13)
config/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-maintain/tests/migrate_integration_test.rscrates/icegate-query/benches/common/harness.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/logql/datafusion/planner_tests.rscrates/icegate-query/tests/loki/harness.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/icegate-maintain/tests/migrate_integration_test.rs
- crates/icegate-query/benches/common/harness.rs
- config/helm/icegate/templates/configmap-ingest.yaml
- config/helm/icegate/templates/configmap-query.yaml
- config/helm/icegate/values.yaml
- crates/icegate-query/src/cli/commands/run.rs
- config/kustomize/overlays/skaffold/values-icegate.yaml
- Ensured `io_cache.close()` is always called to handle Foyer's background flusher tasks gracefully. - Extracted `run_services` function to improve modularity and error handling. - Reduced `memorySizeMb` from 1024 to 512 in Kustomize values for optimized resource usage. - Used `saturating_mul` in `max_write_cache_size_mb` calculation to prevent overflow.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/icegate-ingest/src/cli/commands/run.rs (1)
323-335:⚠️ Potential issue | 🔴 Critical
wal_runtimemay panic when dropped in async context.The
wal_runtimeis created as a local variable insiderun_services(), an async function. When the function returns normally or via?, the runtime is dropped from an async context. Tokio'sRuntime::drop()is a blocking operation that can panic with "Cannot drop a runtime in a context where blocking is not allowed."The
spawn_shift_runtime()pattern (lines 81-131) correctly handles this by spawning the runtime on a dedicated thread with explicit shutdown. Consider applying the same pattern forwal_runtime, or explicitly shut it down before it goes out of scope:💡 Minimal fix using explicit shutdown
server_result?; shift_result?; writer_result??; + + // Shutdown the WAL runtime on a blocking thread to avoid panic + tokio::task::spawn_blocking(move || drop(wal_runtime)) + .await + .map_err(IngestError::Join)?; Ok(()) }Alternatively, refactor
wal_runtimeto use the same thread-owned handle pattern asShiftRuntimeHandle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-ingest/src/cli/commands/run.rs` around lines 323 - 335, The local wal_runtime is created inside async run_services() and will be dropped in an async context (risking a panic); replace the local Runtime with the same thread-owned pattern used by spawn_shift_runtime/ShiftRuntimeHandle (i.e., move wal_runtime creation onto a dedicated thread and return a handle that you explicitly shut down), or if you prefer a minimal change ensure you call wal_runtime.shutdown_timeout(...) or otherwise block/shutdown it synchronously before leaving run_services(); update usages that reference wal_runtime (and the writer.start(write_rx) block) to use the new ShiftRuntimeHandle-style API so the runtime is not dropped inside async code.
🧹 Nitpick comments (1)
crates/icegate-query/tests/loki/harness.rs (1)
104-110: Consider callingstart_background_refresh()for test reliability.The
QueryEngineis created butstart_background_refresh()is not called. When tests append data viawrite_test_logsafter the server starts, the engine may serve stale results until themax_agethreshold forces a synchronous rebuild. For test reliability, consider starting the background refresh:💡 Suggested fix
let query_engine = Arc::new(QueryEngine::new( Arc::clone(&catalog), engine_config, wal_store, wal_reader, )); +query_engine.start_background_refresh();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/icegate-query/tests/loki/harness.rs` around lines 104 - 110, The QueryEngine is constructed but never started, which can serve stale results when tests call write_test_logs; after creating the Arc<QueryEngine> (the variable query_engine) call its start_background_refresh() to kick off the periodic refresh task so the engine sees appended WAL data promptly (ensure you call start_background_refresh() on the same Arc<QueryEngine> instance used by the test server before performing write_test_logs; this will honor the configured max_age and avoid waiting for synchronous rebuilds).
🤖 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-ingest/src/cli/commands/run.rs`:
- Around line 323-335: The local wal_runtime is created inside async
run_services() and will be dropped in an async context (risking a panic);
replace the local Runtime with the same thread-owned pattern used by
spawn_shift_runtime/ShiftRuntimeHandle (i.e., move wal_runtime creation onto a
dedicated thread and return a handle that you explicitly shut down), or if you
prefer a minimal change ensure you call wal_runtime.shutdown_timeout(...) or
otherwise block/shutdown it synchronously before leaving run_services(); update
usages that reference wal_runtime (and the writer.start(write_rx) block) to use
the new ShiftRuntimeHandle-style API so the runtime is not dropped inside async
code.
---
Nitpick comments:
In `@crates/icegate-query/tests/loki/harness.rs`:
- Around line 104-110: The QueryEngine is constructed but never started, which
can serve stale results when tests call write_test_logs; after creating the
Arc<QueryEngine> (the variable query_engine) call its start_background_refresh()
to kick off the periodic refresh task so the engine sees appended WAL data
promptly (ensure you call start_background_refresh() on the same
Arc<QueryEngine> instance used by the test server before performing
write_test_logs; this will honor the configured max_age and avoid waiting for
synchronous rebuilds).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ee21cb6-477c-409a-b38c-c09ba2dc2057
📒 Files selected for processing (13)
config/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-maintain/tests/migrate_integration_test.rscrates/icegate-query/benches/common/harness.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/logql/datafusion/planner_tests.rscrates/icegate-query/tests/loki/harness.rs
✅ Files skipped from review due to trivial changes (2)
- crates/icegate-maintain/tests/migrate_integration_test.rs
- crates/icegate-query/src/logql/datafusion/planner_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/icegate-query/benches/common/harness.rs
- config/helm/icegate/templates/configmap-query.yaml
- config/helm/icegate/templates/configmap-ingest.yaml
- crates/icegate-common/src/catalog/builder.rs
- crates/icegate-common/src/catalog/config.rs
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/catalog/builder.rs`:
- Around line 76-89: The code currently preserves prefetch even when cache is
disabled; update the builder to reject prefetch when there is no cache by
checking cache_config (or the computed cache value) and returning an error if
prefetch is Some while cache is None, rather than keeping prefetch; locate the
logic around cache_config, max_write_cache_size, cache (built via
build_storage_cache) and prefetch in the builder and either return an Err
describing "prefetch requires cache" or strip prefetch to None—preferably return
an error so create_operator (which expects prefetch to flow through the cache)
isn't given an invalid configuration.
In `@crates/icegate-ingest/src/cli/commands/run.rs`:
- Around line 323-335: The wal_runtime created by compute_runtime_threads() and
assigned to wal_runtime is dropped inside the async run_services context which
will panic; fix by making wal_runtime owned and shut down off the async context:
either create and run it inside a dedicated thread like
spawn_shift_runtime/shift_runtime (spawn a thread that builds wal_runtime, calls
wal_runtime.enter()/writer.start(write_rx) there and then calls
wal_runtime.block_on(...) and wal_runtime.shutdown_timeout(...) before thread
exit) or, if you must keep it local, call
wal_runtime.shutdown_timeout(Duration::from_secs(...)) (or
wal_runtime.shutdown_background()/shutdown_on_idle as appropriate) synchronously
before run_services returns so the runtime is not dropped from within an async
context; update references around wal_runtime, writer.start, and run_services to
reflect the chosen strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab27cf33-18f0-4841-90b1-d945bd6bbcc1
📒 Files selected for processing (13)
config/helm/icegate/templates/configmap-ingest.yamlconfig/helm/icegate/templates/configmap-query.yamlconfig/helm/icegate/values.yamlconfig/kustomize/overlays/aws-glue/values-icegate.yamlconfig/kustomize/overlays/skaffold/values-icegate.yamlcrates/icegate-common/src/catalog/builder.rscrates/icegate-common/src/catalog/config.rscrates/icegate-ingest/src/cli/commands/run.rscrates/icegate-maintain/tests/migrate_integration_test.rscrates/icegate-query/benches/common/harness.rscrates/icegate-query/src/cli/commands/run.rscrates/icegate-query/src/logql/datafusion/planner_tests.rscrates/icegate-query/tests/loki/harness.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/icegate-query/benches/common/harness.rs
- crates/icegate-query/tests/loki/harness.rs
- config/helm/icegate/templates/configmap-ingest.yaml
- config/kustomize/overlays/aws-glue/values-icegate.yaml
- crates/icegate-common/src/catalog/config.rs
s-prosvirnin
approved these changes
Mar 24, 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
Improvements