Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/planning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,10 @@ pub async fn distribute_stages(
.collect();

for attempt in 0..3 {
if workers.is_empty() {
return Err(anyhow!("No workers available to distribute stages").into());
}
Comment on lines +454 to +456
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have it for error kinds that can't happen in single node datafusion like WorkerCommunicationError

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// all stages to workers
let (task_datas, final_addrs) =
assign_to_workers(query_id, &stages, workers.values().collect())?;
Expand Down
Loading