[manager] fair eviction across instances within an instance group

[oldsharp]

Fair Eviction Across Instances Within an Instance Group

Context

CacheReclaimer currently runs a per-group cron: when an
InstanceGroup exceeds its watermark (used_percentage of
InstanceGroupQuota), it iterates every instance in the group
and evicts up to a fixed batching_size_ keys per instance per round
(cache_reclaimer.cc:1247-1252). All instances are treated equally
regardless of how much each one is using.

This is unfair when usage is skewed: an instance using <1% of the
group budget can have its hot blocks evicted at the same rate as an
instance using >90%. The aggressor effectively penalises the victim.

Goal: when a group is over-watermark, bias eviction toward the
instances that contribute the most absolute usage to the over-watermark
state. Heaviest contributors are evicted with the largest per-round
batch; instances with zero usage are skipped.

Design Summary

Pure-absolute-usage proportional batching:

• Keep batching_size_ as the per-instance average; redistribute
  it across instances of the group weighted by absolute usage
• Global per-round budget = batching_size_ * N (N = instance count)
  — preserves current throughput
• Per-instance batch = round(global_budget * used_i / sum(used)),
  floored to 1 if used_i > 0, capped at kSizeLimit - 1
• Instances with used_i == 0 are skipped entirely (no work to do)
• Per-storage-type watermark: when only a storage type is exceeded,
  weight by that type's per-instance usage; when both general and a
  type are exceeded, take the per-instance max of the two weights

Behavior is gated by a new CacheReclaimStrategy.enable_instance_fairness
flag, defaulting to true (opt-out). When false, all instances
receive batching_size_ (legacy round-robin).

[qoderai]

Review Summary

Overall impression: Well-designed feature with a clean implementation. The proportional-eviction planner is logically sound, preserves backward compatibility via the enable_instance_fairness flag, and the test coverage is thorough with good edge-case handling (zero-sum fallback, floor-at-one, type-only exceed).

Highlights

• Clean separation between the plan computation (ComputeInstanceReclaimPlan) and the eviction loop — easy to reason about and test in isolation.
• Moving GroupUsageData to the header enables direct unit testing of the planner without full integration wiring.
• The per-instance InstanceUsage breakdown in GroupUsageData avoids a second MetaIndexer pass — efficient.
• Good defensive handling: null instances skipped, zero-sum fallback to legacy, floor at 1 for non-zero contributors.

Key Concerns

1. Protobuf default mismatch (medium): The in-memory default for enable_instance_fairness is true, but the proto bool defaults to false when unset. Older protos or clients that don't set this field will silently disable fairness. See inline comment for mitigation options.

2. Enum contiguity assumption (low): The weight_of lambda casts a raw array index to DataStorageType. If future enum additions break contiguity, this will silently mis-identify types. Iterating over known types (like GetGroupUsageData does) would be more robust.

Minor Notes

• std::map → std::unordered_map for usage_by_id would be a marginal perf win for large instance groups.
• The long double precision concern is purely theoretical on x86-64 Linux but noted for portability.

Tests

The 6 new ComputeInstanceReclaimPlan_* tests cover the key scenarios well: proportional allocation, zero-usage skip, type-only exceed weighting, disabled-flag legacy path, floor-at-one, and zero-sum fallback. The existing TestTriggerReclaiming* tests were correctly adapted to the new GetWaterLevelExceed signature.

---

🤖 Generated by Qoder

[qoderai]

nit: long double sum_w is computed from std::size_t values. On most platforms where long double = 80-bit extended or IEEE 128-bit, this is fine, but worth noting that std::size_t is 64-bit and long double on some platforms (e.g., MSVC/Windows or some ARM) may only have 64-bit mantissa precision — in that case, extremely large sums could lose precision.

Given this code runs on Linux (80-bit extended), this is acceptable. Just flagging for portability awareness if this is ever compiled on non-x86 targets.

---

🤖 Generated by Qoder

[qoderai]

Potential overflow: batching_size * instance_infos.size() could overflow a std::size_t if batching_size is near kSizeLimit (65535) and instance_infos.size() is large (e.g., 2^48 instances). In practice instance counts will be far below what's needed to overflow, but given that batching_size is already validated to be < kSizeLimit, the practical risk is negligible. Just noting for completeness.

---

🤖 Generated by Qoder

[qoderai]

Consider using std::unordered_map instead of std::map for usage_by_id. Since the lookup is by instance_id (a string) and ordering is not needed, an unordered map would reduce the lookup from O(log n) to amortized O(1) per instance. This matters if instance groups grow to hundreds of instances.

std::unordered_map<std::string, const GroupUsageData::InstanceUsage *> usage_by_id;
usage_by_id.reserve(group_usage_data.per_instance_.size());

---

🤖 Generated by Qoder

[qoderai]

The cast static_cast<DataStorageType>(idx) converts a raw index into a DataStorageType enum value. This relies on the enum values being contiguous integers starting from 0, which is currently true based on the array layout. However, if a new DataStorageType is added with a non-sequential value in the future, this cast would silently produce an incorrect type.

Consider adding a static assertion or a compile-time check that the enum is sequential, or alternatively iterate only over the known storage types (similar to how GetGroupUsageData does it with explicit calc_sz() calls).

---

🤖 Generated by Qoder

[qoderai]

Protobuf bool fields default to false when unset. This means that if a client sends a CacheReclaimStrategy proto message without explicitly setting enable_instance_fairness (e.g., an older client or admin tool), it will be deserialized as false — disabling the feature.

This conflicts with the in-memory default (enable_instance_fairness_ = true). Consider either:

1. Treating the protobuf false default as acceptable (documentation that older protos disable fairness), or
2. Using a wrapper type (e.g., google.protobuf.BoolValue) in the proto so you can distinguish "not set" from "explicitly false" and apply the in-memory default when not set.

---

🤖 Generated by Qoder