DAP-16 Leader Job State Updates#4324
DAP-16 Leader Job State Updates#4324jcjones wants to merge 1 commit intojcj/4322-dap16-helper-job-state-updatesfrom
Conversation
Leader jobs should be transitioned from Active to Finished by the Driver, not by the Writer. Fixes #4320
| Extension::new(ExtensionType::Reserved, Vec::new()), | ||
| Extension::new(ExtensionType::Reserved, Vec::new()), |
There was a problem hiding this comment.
I think this is overkill in terms of inducing an error. We would normally reject this report earlier in the process on the leader side, both because there are two extensions with the same type, and we don't support this particular extension type. It should be sufficient to just have the mock helper response contain ReportError::InvalidMessage. Additional problems here might cause problems in the future if we get stricter about what related structs can contain.
| ); | ||
|
|
||
| // Determine the next Leader job state based on whether all reports are terminal. | ||
| let all_terminal = report_aggregations_to_write |
There was a problem hiding this comment.
Same observations as in #4323: if we're doing this here, we shouldn't do it again in the aggregation job writer. It seems like we consider it a safety net, but we should have enough confidence in our state transitions to avoid redundancy.
|
After trimming off the state-transition change, there's nothing left of this PR, so I am closing it. |
3 participants
Leader jobs should be transitioned from Active to Finished by the Driver, not by the Writer.
Fixes #4320.
Note
Stacked on #4322.