andygrove commented on PR #2364: URL: https://github.com/apache/datafusion-ballista/pull/2364#issuecomment-5442474704
This looks good to me. One ask and one suggestion. **Upgrade guide.** The `BALLISTA_PROTOCOL_VERSION` bump from 2 to 3 is the part I care most about. Anyone doing a rolling upgrade is going to see executors rejected at registration, and `docs/source/upgrading/55.0.0.md` is the only place they'd think to look for why. A short note under the behavior changes section saying schedulers and executors have to be upgraded together, and that the mismatch shows up as a registration rejection rather than a silent misread, would cover it. The strict equality check is the right design and I'd rather it be documented than discovered. While you're in that file, the `### API changes` section around line 100 could take the public breaks in `ballista-core`: - `Action::FetchPartition` loses `is_sort_shuffle` and gains `layout`, `file_kind`, and `byte_ranges` - `BallistaClient::fetch_partition` and `fetch_partition_proxied` take a `ShuffleLayout` where they took a bool - `BallistaClient::execute_do_action` gains a `header` parameter The proto side of that is fine as it stands, reserving field 8 and taking a fresh number for `layout` is the right call. Could you also add the `api change` label. **A thought on `execute_do_action`.** Every existing caller passes `None` for the new parameter, so this one could stay source compatible: add `execute_do_action_with_header(action, header)` and have `execute_do_action(action)` delegate with `None`. The other breaks here are unavoidable, a struct variant can't gain a field without breaking construction and the layout enum genuinely replaces the bool, but this one looks like a free save. Entirely your call if you'd rather keep the single method. Might also be worth marking `Action::FetchPartition` `#[non_exhaustive]` while you're here, so the next field added to it isn't a break either. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
