Add guard to avoid panic #44
Conversation
| if workers.is_empty() { | ||
| return Err(anyhow!("No workers available to distribute stages").into()); | ||
| } |
There was a problem hiding this comment.
🤔 I wonder why we have our own custom DDError wrapping other kinds of errors. I would have expected the vanilla DataFusionError from upstream to be sufficient for representing what we are doing.
Nothing related to this PR though.
There was a problem hiding this comment.
I think we have it for error kinds that can't happen in single node datafusion like WorkerCommunicationError
There was a problem hiding this comment.
@gabotechs regarding the error, my thinking was twofold. When returning an error to a client, those errors may be sourced by Arrow, DataFusion, Flight, or something else. I wanted to account for them all. Also when writing a function that returns a Result, if more than one type of error can occur in that function, its convenient (maybe its lazy?) to return a result type that has From traits for both of those errors so you can just use ? or into to propagate them.
There was a problem hiding this comment.
And can't all of that be accomplished with just pure DataFusionError? upstream has:
- DataFusionError::ArrowError
- DataFusionError::Execution
- DataFusionError::IoError
- DataFusionError::External(GenericError)
I imagine that for when this is open source, folks using datafusion-distributed could appreciate not needing to deal with additional error structures.
assign_to_workerswill panic when no healthy workers remain in the set.