Skip to content

[SPARK-41530][CORE] Rename MedianHeap to PercentileMap and support percentile#39076

Closed
cloud-fan wants to merge 1 commit intoapache:masterfrom
cloud-fan:percentile
Closed

[SPARK-41530][CORE] Rename MedianHeap to PercentileMap and support percentile#39076
cloud-fan wants to merge 1 commit intoapache:masterfrom
cloud-fan:percentile

Conversation

@cloud-fan
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

MedianHeap was added to track the median of task durations, for task speculation. However, median may not be the best option and a configurable percentile could be better.

This PR extends the existing MedianHeap to support percentile.

Why are the changes needed?

Prepare for tracking more statistics of task durations.

Does this PR introduce any user-facing change?

no

How was this patch tested?

new tests

@cloud-fan cloud-fan marked this pull request as ready for review December 15, 2022 12:19
@github-actions github-actions bot added the CORE label Dec 15, 2022
@cloud-fan
Copy link
Copy Markdown
Contributor Author

cc @Ngone51

@mridulm
Copy link
Copy Markdown
Contributor

mridulm commented Dec 15, 2022

Prepare for tracking more statistics of task durations.

Can you give more details on this please ?

Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Technically, MedianHeap is removed by being superceded by PercentileHeap after this PR.

  • Since new class is important than the removed one, could you revise the PR title to mention PercentileHeap explicitly?
  • Also, in the test suite, I believe it would be better not to mention mediaHeap.

@cloud-fan
Copy link
Copy Markdown
Contributor Author

Can you give more details on this please ?

Using the median task duration to trigger task speculation may not be the best option. It's better to allow users to configure the percentile if they want task speculation to happen more or less likely.

@cloud-fan cloud-fan changed the title [SPARK-41530][CORE] Extend MedianHeap to support percentile [SPARK-41530][CORE] Rename MedianHeap to PercentileMap and support percentile Dec 16, 2022
@mridulm
Copy link
Copy Markdown
Contributor

mridulm commented Dec 16, 2022

Using the median task duration to trigger task speculation may not be the best option. It's better to allow users to configure the percentile if they want task speculation to happen more or less likely.

Sounds good.
Note that given the complexity of how we determine whether a task is speculatable or not, it is not just a matter of turning the dial on the percentile :-)

Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @cloud-fan .

@dongjoon-hyun
Copy link
Copy Markdown
Member

Merged to master for Apache Spark 3.4. Thank you, @cloud-fan and @mridulm .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants