lib/collection/src/shards/queue_proxy_shard.rs (1)

748-764: 🛠️ Refactor suggestion

Propagate the refined wait logic to the per-operation fallback

Following the earlier remark, the loop can be adjusted:

-for (_idx, operation) in batch {
+for (i, (_idx, operation)) in batch.iter().enumerate() {
     let mut operation = operation.clone();
@@
-        remote_shard
+        remote_shard
             .forward_update(
                 operation,
-                wait,
+                wait_for_last_op_only && i == batch.len() - 1,
                 WriteOrdering::Weak,
                 hw_measurement_acc.clone(),
             )

This keeps the non-batched path behaviourally identical to the new batched API in terms of client-perceived durability.

lib/api/src/grpc/proto/points_internal_service.proto (2)

100-114: Add reserved numbers / names for forward compatibility

Once shipped, the field numbers inside UpdateOperation are frozen.
If a variant is ever removed/renamed we will not be able to reuse its tag.
Reserve a small range now to avoid a breaking change later:

message UpdateOperation {
  reserved 12 to 20;       // keep room for future variants
  oneof update {
      // …
  }
}

Also consider using distinct message types for set_payload vs overwrite_payload
to prevent accidental misuse by code-gen tools that rely on variant names rather
than numbers.

---

117-119: Nit: pluralise operations or rename wrapper

The wrapper is called UpdateBatchInternal yet the field inside is already
operations. For terseness & protobuf style consistency this could be:

-message UpdateBatchInternal {
-    repeated UpdateOperation operations = 1;
-}
+message UpdateBatchInternal {
+    repeated UpdateOperation ops = 1;
+}

Purely cosmetic, but keeps request payloads a few bytes smaller and in line with
existing messages like search_points, query_points, etc.

lib/collection/src/shards/remote_shard.rs (3)

105-112: check_version may return stale/unknown results – clarify semantics

peer_is_at_version looks like a cache lookup. If the peer hasn’t announced its
version yet we’ll return false, which the caller may interpret as “feature
unsupported” and silently fall back to the slow path. That defeats the purpose
of the optimisation.

Consider returning an enum (Unknown | Below | AtLeast) or exposing a
refresh_version helper so the caller can trigger an explicit health-check
when the version is unknown.

---

229-237: Potentially large allocation – pre-size updates correctly

updates is initialised with operations.len() capacity – good – but we
immediately iterate by value, causing another allocation when converting
to Protobuf structs.
If operations is large this doubles memory usage.

- let mut updates = Vec::with_capacity(operations.len());
+ let mut updates = Vec::with_capacity(operations.len()); // keep
+ updates.reserve(operations.len());                      // not needed, remove comment

// OR build in-place with `.map().collect()`:
+ let updates: Vec<UpdateOperation> = operations.into_iter().map(|operation| {
+     // conversion
+ }).collect();

---

242-415: Method doing too much – extract conversion logic

forward_update_batch contains ~160 lines of nested match statements making
it hard to read and maintain. Extract the “local operation ➜ protobuf
Update” mapping into a dedicated helper:

fn into_update_proto(
    op: OperationWithClockTag,
    shard_id: ShardId,
    collection: &str,
    wait: bool,
    ordering: Option<WriteOrdering>,
) -> CollectionResult<Update> { … }

Then:

let updates: Result<Vec<_>, _> = operations
    .into_iter()
    .map(|op| into_update_proto(op, …))
    .try_collect()?;

This shortens the method, improves testability, and keeps each concern in its
own place.

lib/api/src/grpc/qdrant.rs (4)

9334-9345: Derive Deserialize and align JSON naming to avoid inter-service incompatibilities
The new UpdateOperation is only Serialize-able. Any component that might consume batched requests or log them in JSON now has no straightforward way to de-serialize them back (e.g. for re-play, testing or audit). This is especially painful once the batch travels through message queues or is cached.

-#[derive(serde::Serialize)]
+#[derive(serde::Serialize, serde::Deserialize)]
+#[serde(rename_all = "snake_case")]

(The same change is advisable for every struct / enum that is exposed outside the binary.)

Adding Deserialize is zero-cost while future-proofing the API.

---

9381-9384: Implement Default for convenience & testability
UpdateBatchInternal is frequently constructed in unit / integration
tests. Deriving Default (cheap because all fields already implement
it) streamlines that workflow and avoids verbose ...::default()
wrappers.

-#[derive(Clone, PartialEq, ::prost::Message)]
+#[derive(Clone, PartialEq, ::prost::Message, Default)]

---

10530-10536: Add minimal rust-doc for the new trait method
PointsInternal::update_batch is a public contract but ships without
documentation. Two-line docs explaining ordering guarantees and error-
propagation will save future readers a trip through the implementation.

---

11172-11217: Beware of very large batches – expose max message size knobs
The server handler uses the global max_decoding_message_size, but
client code defaults to tonic’s 4 MiB limit. For delta WAL replication,
a single batch can easily exceed that. Consider:

1. Documenting the recommended limits in README / config.
2. Allowing per-request overrides (update_batch(max_size=…)).
3. Emitting a clear error when the limit is hit so operators know which
   knob to tweak.

This avoids mysterious ResourceExhausted: received message larger than max errors in production.

lib/collection/src/shards/queue_proxy_shard.rs (1)

719-746: Avoid re-computing feature-flag per batch

remote_shard.check_version(&MINIMAL_VERSION_FOR_BATCH_WAL_TRANSFER) is evaluated for every WAL batch.
For long catch-ups this extra RPC / parse can add noticeable overhead.

Suggestion:

1. Cache the boolean once in Inner (e.g. a supports_batch_updates field initialised during the first transfer).
2. Re-use the cached flag inside transfer_operations_batch.

This tiny change keeps the critical path free of redundant version checks.

src/tonic/api/points_internal_api.rs (2)

561-632: time metric for update_batch only reflects the last operation

last_result.time is left untouched after aggregating multiple operations, so the response under-reports
the wall-clock cost when the batch contains >1 update.

Consider accumulating the timings similarly to usage:

-let mut total_usage = HardwareUsage::default();
+let mut total_usage = HardwareUsage::default();
+let mut total_time   = 0.0_f64;
@@
-        let mut response = result.into_inner();
+        let mut response = result.into_inner();
+
+        total_time += response.time;
@@
-            last_result = Some(response)
+            last_result = Some(response)
@@
-if let Some(mut last_result) = last_result.take() {
-    last_result.usage = Some(total_usage);
+if let Some(mut last_result) = last_result.take() {
+    last_result.usage = Some(total_usage);
+    last_result.time  = total_time;

This gives clients an accurate latency picture while preserving backward-compatibility.

---

438-447: Nit: unnecessary string clone on every call

get_request_collection_hw_usage_counter_for_internal takes collection_name: String by value,
forcing each caller to allocate/clamp a new String. A lightweight &str suffices:

-fn get_request_collection_hw_usage_counter_for_internal(
-    &self,
-    collection_name: String,
-) -> RequestHwCounter {
-    let counter = HwMeasurementAcc::new_with_metrics_drain(
-        self.toc.get_collection_hw_metrics(collection_name),
+fn get_request_collection_hw_usage_counter_for_internal(
+    &self,
+    collection_name: &str,
+) -> RequestHwCounter {
+    let counter = HwMeasurementAcc::new_with_metrics_drain(
+        self.toc.get_collection_hw_metrics(collection_name.to_owned()),
     );

and call-sites become:

- self.get_request_collection_hw_usage_counter_for_internal(collection_name.clone());
+ self.get_request_collection_hw_usage_counter_for_internal(&collection_name);

This removes a few pointless heap allocations on a hot path.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro