enhance: [2.6] add max loading size config for caching layer - #121
enhance: [2.6] add max loading size config for caching layer#121sparknack wants to merge 2 commits into
Conversation
|
|
||
| manager.dlist_ = | ||
| std::make_shared<internal::DList>(eviction_enabled, max, low_watermark, high_watermark, eviction_config); | ||
| manager.dlist_ = std::make_shared<internal::DList>(options.eviction_enabled, max, low_watermark, high_watermark, |
There was a problem hiding this comment.
[P1] Honor the loading cap when eviction is disabled
This wires max_loading_size into DList, but normal CacheSlot loads still set self_reserve solely from eviction_enabled_; RunLoad bypasses ReserveLoadingResourceWithTimeout when that flag is false. As a result, a finite loading cap is silently ignored for normal cache loads whenever eviction is disabled, which is also the default configuration. Decouple loading admission from eviction, or reject and document this unsupported combination instead of accepting an ineffective cap.
There was a problem hiding this comment.
This issue still exists: Manager::CreateCacheSlot still derives self_reserve solely from eviction_enabled_, and CacheSlot::RunLoad still bypasses DList reservation when that flag is false, so a finite loading cap remains ineffective in that configuration.
| Manager::UpdateConfig(std::chrono::milliseconds loading_timeout, std::chrono::milliseconds warmup_loading_timeout, | ||
| bool storage_usage_tracking_enabled, CacheWarmupPolicies warmup_policies, | ||
| ResourceUsage max_loading_size) { | ||
| TieredStorageConfig::GetInstance().UpdateAll(storage_usage_tracking_enabled, loading_timeout, |
There was a problem hiding this comment.
[P1] Make max-loading updates atomic across owners
max_loading_size is now stored in both TieredStorageConfig and DList, but this overload publishes the snapshot and enforcement value in separate critical sections. Two overlapping updates can apply in different orders—for example, A writes config A, B writes config and DList B, then A writes DList A—leaving snapshots at B while admission enforces A indefinitely. The public config-only mutators can also change the snapshot without changing enforcement. Keep one authoritative value, or serialize/version the composite update and prevent config-only mutation.
There was a problem hiding this comment.
This issue still exists: the ratio is still published in TieredStorageConfig before the enforcement value is updated in DList, so concurrent updates or direct config mutators can leave the two owners out of sync.
| waiting_requests_map_.erase(request->request_id); | ||
| requests_to_destroy.push_back(std::move(request)); | ||
| waiting_queue_.pop(); | ||
| } else if (canReserveByMaxLoadingSize(request_ptr_ref->required_size) && |
There was a problem hiding this comment.
[P2] Wake followers after a capped waiter expires
The new cap can leave the queue head blocked here after a partial release even though a smaller follower fits the remaining loading budget. If that head then times out or is cancelled, the callbacks only fulfill its promise and erase it from waiting_requests_map_; they neither remove the stale heap entry nor call handleWaitingRequests(). With background eviction disabled, the fitting follower is not retried until another unrelated release or config update and can time out unnecessarily. Drain stale entries and retry the queue when a waiter is failed or cancelled.
There was a problem hiding this comment.
This issue still exists: timeout and cancellation callbacks still fulfill the request and erase only its lookup-map entry without draining the heap or retrying followers.
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
1e394db to
e2596ac
Compare
sparknack
changed the title
| DList::exceedMaxLoadingMemSize() const { | ||
| const auto max_loading_mem_size = max_loading_mem_size_.load(); | ||
| const auto total_loading_size = total_loading_size_.load(); | ||
| return max_loading_mem_size >= 0 && total_loading_size.memory_bytes >= max_loading_mem_size; |
There was a problem hiding this comment.
[P1] Enforce the cap against projected loading memory
This predicate compares only the already-reserved loading memory with the cap. While the current value is below the limit, reserveResourceInternal() admits the entire scaled request even if current + request exceeds it; for example, a 500 MiB request is accepted when 900 MiB is already loading under a 1 GiB cap, and a single request larger than the cap is accepted from zero. Once the cap is reached, the same predicate also blocks disk-only requests that add no memory. Pass the scaled request size into the check, compare projected memory usage, and explicitly handle requests that can never fit so this setting is an actual upper bound.
| AssertInfo(max_loading_mem_ratio >= 0 && max_loading_mem_ratio <= 1, | ||
| "[MCL] max loading memory ratio must be between 0 and 1, got {}", max_loading_mem_ratio); | ||
|
|
||
| const auto total_memory = getSystemMemoryInfo().total_bytes; |
There was a problem hiding this comment.
[P2] Use the cgroup limit when host memory is unavailable
getMaxLoadingMemSize() now relies on getSystemMemoryInfo().total_bytes, but that helper assigns host_memory when host_memory == 0 and container_limit > 0, because the container_limit < host_memory branch cannot be taken. In a restricted container where /proc/meminfo cannot be read but the cgroup limit is available, this produces zero and this function returns -1, silently disabling the configured loading cap. Select whichever positive memory source is available before applying the ratio.
2 participants
No description provided.