RexXiong commented on PR #3686: URL: https://github.com/apache/celeborn/pull/3686#issuecomment-4504657894
Nice optimization! The pre-computed usable slots and the removal of LinkedList iterator overhead are solid wins — the benchmark numbers (63-155 ns/partition) confirm the approach works well for large partition counts. A few concerns: **1. Rolling upgrade compatibility: client must not send V2 to an old master** The serialization now always emits `REQUEST_SLOTS_V2`. During a rolling upgrade where the client (LifecycleManager) is upgraded before the master, the old master will fail to deserialize the V2 message — it doesn't have the `REQUEST_SLOTS_V2_VALUE` case branch yet. Suggested approach: either (a) document that master must be upgraded before clients (and enforce via version check), or (b) have the client detect master version and fall back to V1 serialization when talking to an older master. **2. Iteration direction change — allocation order reversed** The original code iterated partitions from the **end** of the list (`listIterator(size)` + `hasPrevious()`), allocating higher-numbered partitions first. The new code iterates from the **beginning**, allocating lower-numbered partitions first. This means the worker-to-partition mapping is different given the same inputs. While this likely has no functional impact (partition IDs are just identifiers for slot allocation), it's a behavioral change that could affect allocation patterns in subtle ways (e.g., which partitions land on which workers during partial allocation). Worth documenting in the PR description that this behavioral change is intentional and benign. **3. Minor: `computeUsableSlots` returns `long[]` but only needs `int[]`** The usable slot counts per worker fit comfortably in `int` (max slots per worker disk is bounded). Using `long[]` wastes 2x memory for large worker arrays. Consider `int[]` instead — cleaner and avoids any confusion about why `long` was chosen (reader might wonder if values can exceed Integer.MAX_VALUE). --- Overall the optimization is well-structured and the benchmark provides good evidence. The V2 upgrade path is the main concern. *Reviewed with Claude Code* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
