You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sandugood (via GitHub)" <gi...@apache.org> on 2026/06/09 21:42:25 UTC
[PR] feat: decouple partition's location from executor's metadata [datafusion-ballista]
sandugood opened a new pull request, #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853
# Which issue does this PR close?
Closes #1851.
# Rationale for this change
In the current implementation there is a problem - in the `PartitionLocation` that is used in each shuffle operation (for each partition in the previous stage) there was `executor_metadata` field (which is of `ExecutorMetadata` type) filled with unnecessary info, because it did not add up any information that could be used by `ShuffleReaderExec` to extract partition (or i.e resume the execution from a failed stage)
# What changes are included in this PR?
`ExecutorMetadata` was decoupled from `PartitionLocation` and now it is used as a separate struct for:
- registering executor
- in heartbeats
- REST API info fetch
`PartitionLocation` now is only exposed to the `executor_id`, host and `port`, which is sufficient for fetching needed partitions by the `ShuffleReaderExec`
This way we can save up a lot of space and data-transfer during each shuffle operation (potentially removing possibility of `Scheduler`'s OOM errors and improving speed of queries with lots of partitions)
# Are there any user-facing changes?
Yes.
In the REST API interface `ExecutorResponse` doesnt contain nested struct with `Executor`'s hardware and OS info. It was flattened.
# Potential follow-up:
- check the TUI REST API integration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4742357916
running with 1024 partitions
```
Error, decoded message length too large: found 51824978 bytes, the limit is: 16777216 bytes
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4669942673
Thank you for your review and ideas @milenkovicm
Going to tackle it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4741128632
I'm running TPCH (SF10) Q2 with 512 partitions
```bash
cargo run --bin tpch -- benchmark ballista -p /Users/marko/TMP/tpch_data/tpch-data-sf10/ -f parquet -i 1 --port 50050 --host 127.0.0.1 -c datafusion.execution.target_partitions=512 -q 2
```
before PR
```
Error, decoded message length too large: found 43952013 bytes, the limit is: 16777216 bytes
```
after PR has been applied
```
Error, decoded message length too large: found 18947588 bytes, the limit is: 16777216 byte
```
so its like 2.5x improvement which is good direction
I know there are other things we have to do in order to make using even less data like proposed in #1876 using uuid instead of strings 🤔
good progress @sandugood any suggestions @coderfender ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4758778865
Refactored the code and placed it inside the `PartitionLocationExt` .proto spec. Seemed like a good reason to save some bytes here and there.
However, by running the same Q-2 query on TPCH SF-2 - `cargo run --bin tpch -- benchmark ballista -p /home/sandu/datafusion-ballista/data -f parquet -i 1 --port 50050 --host 127.0.0.1 -c datafusion.execution.target_partitions=512 -q 2`
Got:
`Error, decoded message length too large: found 18834462 bytes, the limit is: 16777216 bytes`
Saved 100k (comparing to previous test report) by this change, but couldn't fit it inside the default threshold.
cc @milenkovicm @coderfender
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4741369002
we might not be able to save much on top of this changes, and removing `executor_id` from metadata might not be justified to bring new type (especially as number of executors might not be too big) so id suggest to try to encapsulate partition locations and executor maps and then if further optimisations could be done we can take them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4815507874
Apologies @sandugood
Will review this asap AFK for short summer break
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4735551132
@sandugood , @milenkovicm I closed my PR to prioritize this approach which seems to be more promising and pragmatic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4741242583
Yeah, I will definitely dig into that
Saving few bytes here and there makes massive change at scale
Thanks for review @milenkovicm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4972434675
Thanks @milenkovicm
No need to hurry, take your time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on code in PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#discussion_r3428187159
##########
ballista/core/src/serde/scheduler/to_proto.rs:
##########
@@ -78,7 +78,7 @@ impl TryInto<protobuf::PartitionLocation> for PartitionLocation {
Ok(protobuf::PartitionLocation {
map_partition_id: self.map_partition_id as u32,
partition_id: Some(self.partition_id.into()),
- executor_meta: Some(self.executor_meta.into()),
+ executor_id: self.executor_meta.clone().id.clone(),
Review Comment:
Nope, works either way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4963103545
sorry @sandugood for delay, will review this PR in next few days, can you please rebase it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #1853: feat: decouple partition location from executor metadata
URL: https://github.com/apache/datafusion-ballista/pull/1853
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4726995595
@sandugood thank you for the PR. I was wondering if you could provide some sort of before and after benchmarks to gain more confidence here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4729905231
had a quick look, decoupling executor connection information from partition location makes sense. will try to have another look later. thanks @sandugood
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#discussion_r3386621203
##########
ballista/core/proto/ballista.proto:
##########
@@ -367,8 +370,16 @@ message ExecutorMetadata {
string host = 2;
uint32 port = 3;
uint32 grpc_port = 4;
- ExecutorSpecification specification = 5;
Review Comment:
could we make this properties optional rather than flatten them?
##########
ballista/core/proto/ballista.proto:
##########
@@ -272,10 +272,13 @@ message PartitionLocation {
uint32 map_partition_id = 1;
// partition_id of the shuffle, a composition of(job_id + map_stage_id + partition_id).
PartitionId partition_id = 2;
- ExecutorMetadata executor_meta = 3;
PartitionStats partition_stats = 4;
- optional uint64 file_id = 6;
- bool is_sort_shuffle = 7;
+ string executor_id = 6;
+ string host = 7;
+ uint32 port = 8;
+ uint32 grpc_port = 9;
Review Comment:
this is going to be replicated in each and every partition location, which is not optimal if we take into account 1000s of them.
Would it be possible to serialise `PartitionLocation` into actual vector of locations and map of executors, where key is going to be executor_id (as bytes) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4741301084
furthermore,
when running Q2 with 256 partitions, executor takes around 460MB with this patch, and 550MB without (which might not be very good metrics to start with) but what i see
with this PR:
```
Query 2 took 6.989 s and returned 100 rows
Total time: 6.989 s
```
and without:
```
Query 2 took 12.037 s and returned 100 rows
Total time: 12.037 s
```
(unless i made some kind of mess-up with test runs)
Im running latest main with
```
cargo run --bin ballista-executor --release -- -c 4 --work-dir /path/to/work/dir./
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#discussion_r3428123923
##########
ballista/core/src/serde/scheduler/to_proto.rs:
##########
@@ -78,7 +78,7 @@ impl TryInto<protobuf::PartitionLocation> for PartitionLocation {
Ok(protobuf::PartitionLocation {
map_partition_id: self.map_partition_id as u32,
partition_id: Some(self.partition_id.into()),
- executor_meta: Some(self.executor_meta.into()),
+ executor_id: self.executor_meta.clone().id.clone(),
Review Comment:
do we need double clone here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] [WIP] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4717625526
Refactored the code:
1. In the .proto spec now we are sending the `PartitionLocation` along with a map of `ExecutorMetadata` (used by `ShuffleReaderPartition`). So we can now transfer a `Vec<>` of locations and access the executor's metadata via its `id`.
2. On the native side - added `ExecutorMetadata` in the `PartitionLocation` behind an `Arc<>` to avoid heavy copying.
3. Made parts of the .proto spec in the `ExecutorMetadata` optional
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-4727331533
The OOM crash was reproduced by @gabotechs
Wonder if you could potentially test it against this version? Thanks a lot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: decouple partition location from executor metadata [datafusion-ballista]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1853:
URL: https://github.com/apache/datafusion-ballista/pull/1853#issuecomment-5825348771
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org