apache / apache/datafusion-ballista
NewType pattern for executor id's
- Dominant language
- Rust
- Stars
- 2.1k
- Forks
- 320
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 66
Description
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
With the landing of #1852 , the next logical place to create a new type for is `ExecutorId`. This will help making clear when collections are keyed by executor id's or job id's, for example. The approach is similar (or, the same really) as for `JobId`.
**Describe the solution you'd like**
- Create a new type `ExecutorId(String)` (or `ExecutorId(Uuid)`)
- Replace all `executor_id` with this new type.
**Describe alternatives you've considered**
Executor ID's are UUIDs, and it was suggested to making it `ExecutorId(Uuid)` (see this [comment](https://github.com/apache/datafusion-ballista/pull/1852#issuecomment-4673754250)).
There are a few downsides to this that I could think of:
- We would have to change a lot of tests.
- Keeping `ExecutorId` semantically an opaque identifier (as it is now as well) feels more flexible.
- We wouldn't be able to reuse the introduced macro that automatically implements the ergonomic (imo, obviously biased as author) traits such as `From<&str>` and `Borrow`. The latter is important for easy use with `HashMap`s. I could of course make a new macro so this is not a very strong argument.
@martin-g : as the original commenter, what do you think? If you think `ExecutorId(Uuid)` makes more sense, then I will do that.
**Additional context**
This will await #1851 as that work is in all the same places.
Contributor guide
Research direction
Start by reviewing the related JobId work in #1852 and the dependency in #1851, then locate the executor_id usages across the repository. Resolve whether ExecutorId should wrap String or Uuid, replace the relevant usages consistently, and update affected tests. Done means executor identifiers use the newtype without breaking collection lookups or existing behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- distributed-systems
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100