core: Use Runner enum type instead of string for Job model #651
Conversation
feast-dev#575 sought to clear up inconsistencies between uses of `Runner#name()` (the standard final method of `java.lang.Enum` that returns the value's enum constant name) and the riskily-named `Runner#getName()` defined in Feast for human-readable Beam Runner names. The latter is used as runner name users can set in config. The former is used for values of the runner column of the jobs table in SQL (as it should be). But it relied on careful coding to use the right one when constructing `Job` instances. This is error prone, as feast-dev#578 demonstrates. There is a more robust way: use the enum instead of stringly-typed programming. It's one of the reasons we have enums :-) This also renames the internal identifier in the Runner definition to `humanName`, to distinguish it further from `Enum#name()`.
|
/assign @woop |
| return r; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException("Unknown value: " + runner); | ||
| throw new NoSuchElementException("Unknown Runner value: " + humanName); |
There was a problem hiding this comment.
I felt this is perhaps a more fitting exception type for this case.
| // Use Runner.name() when converting a Runner to string to assign to this property. | ||
| @Enumerated(EnumType.STRING) | ||
| @Column(name = "runner") | ||
| private String runner; | ||
| private Runner runner; |
There was a problem hiding this comment.
The removed comment sums up the motivation of the PR.
| private Map<String, JobManager> jobManagers; | ||
| private final JobRepository jobRepository; | ||
| private final SpecService specService; | ||
| private final Map<Runner, JobManager> jobManagers; |
There was a problem hiding this comment.
Out of scope for this PR, but I still feel like JobManager can be changed to something closer to Runner. In fact it can probably be Runner itself. Meaning this code would become private final Map<RunnerType, Runner>
|
/test test-end-to-end-batch |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This is refactoring aiming to remove a burden on the programmer to get something right.
#575 sought to clear up inconsistencies between uses of
Runner#name()(the standard final method ofjava.lang.Enumthat returns the value's enum constant name) and the riskily-namedRunner#getName()defined in Feast for human-readable Beam Runner names.The latter is used as runner name users can set in config (now as
Runner#toStringsince #575). The former is used for values of the runner column of the jobs table in SQL (as it should be). But it relied on careful coding to use the right one when constructingJobinstances. This is error prone, as #578 demonstrates.There is a more robust way: use the enum instead of stringly-typed programming. It's one of the reasons we have enums☺️
This also renames the internal identifier in the Runner definition to
humanName, to distinguish it further fromEnum#name().Which issue(s) this PR fixes:
No issue. Relates to #575, #578, brought up in #650 (comment)
Does this PR introduce a user-facing change?: