pltbkd commented on PR #3718: URL: https://github.com/apache/celeborn/pull/3718#issuecomment-4629543191
> @pltbkd thanks for the review! Agreed the reducer-centric naming reads oddly now that map partitions reuse it. > > The good news is the rename would be **wire-safe**, because the protocol is keyed by numbers, not names: > > * `MessageType.READ_REDUCER_PARTITION_END = 94` / `_RESPONSE = 95` — protobuf encodes the message/enum by tag number, not by name, so renaming to a `READ_PARTITION_END` / `PbReadPartitionEnd` series while keeping `94`/`95` doesn't change the wire format. > * `StatusCode.READ_REDUCER_PARTITION_END_FAILED(56)` — same story: the `56` is what goes on the wire, not the constant name. > > And this RPC is client → `LifecycleManager`, which run at the same Celeborn version within a single application (unlike the worker↔master path), so there's no rolling-upgrade skew to worry about either. The source footprint is small as well (~5 files each for `ReadReducerPartitionEnd` / `READ_REDUCER_PARTITION_END`). > > My only hesitation is mixing a protocol-wide rename (it also touches the existing Spark path) into this Flink feature PR. I'd lean toward doing the `readPartitionEnd` rename as a focused follow-up right after this merges, to keep this diff reviewable — but I'm happy to fold it in here if you'd prefer. WDYT? Sorry for the late reply. Agreed. -- 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]
