Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the newer SqlQueryScheduler #21952

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

rschlussel
Copy link
Contributor

@rschlussel rschlussel commented Feb 16, 2024

Remove the "new" SqlQueryScheduler, which was created to support stage retries, but was never rolled out and is no longer important because of Presto-on-Spark. Rename LegacySqlQueryScheduler to SqlQueryScheduler.

Description

Remove the SqlQueryScheduler and rename LegacySqlQueryScheduler back to SqlQueryScheduler. Remove corresponding session properties that are no longer relevant.

Motivation and Context

The new SqlQueryScheduler, which was built to enable stage retries when materializing exchanges has never been rolled out, and has a bug that causes it to use a lot of cpu that has not been resolved (was disabled here, #14264 and an attempt to fix it here #14879 did not resolve the issue).

The new scheduler was deprioritized with the investment in presto-on-spark. This PR removes the "new" scheduler, which has been tech debt for a while since we are not investing in fixing it and making it stable for production.

The LegacySqlQueryScheduler has been the default, so there is no user facing change except for removing the ability to try out the new scheduler.

Fixes #21886

Impact

Removes ability to use the experimental SqlQueryScheduler that allowed for stage retries

Test Plan

CI

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Remove the configuration property ``use-legacy-scheduler`` and the corresponding session property ``use_legacy_scheduler`.   The property previously defaulted to true, and the new scheduler, which was intended to replace it eventually, was never productionized and is no longer needed. The configuration property ``max-stage-retries`` and the session property ``max_stage_retries`` have also been removed.

@rschlussel rschlussel requested a review from a team as a code owner February 16, 2024 20:05
@kaikalur
Copy link
Contributor

If it is a useful/functional piece we should keep it. Who knows maybe we will find use for it!

@rschlussel
Copy link
Contributor Author

If it is a useful/functional piece we should keep it. Who knows maybe we will find use for it!

The problem is that it's buggy and it's not clear how much effort it would take to be confident enough to use it in production. If the feature were important for us now, i would 100% say let's keep it and take it that last mile. But it's not really a priority, so in the meantime it's tech debt that makes it harder to add features to the scheduler (you need to add everything in two places in case one day someone decides we should actually productionize and use the new scheduler).

@tdcmeehan
Copy link
Contributor

If it comes in handy later, we can simply recover it from Git history. I agree with @rschlussel that there's costs to maintaining this class, and while for this one class it might be small, dead code over the whole project it is a significant technical burden and I think we should err on the side of removing unused functionality.

ajaygeorge
ajaygeorge previously approved these changes Feb 17, 2024
Copy link
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

LGTM. Please take a look at the failing tests as well.

"\n" +
"a. Print VLOG(2) and lower messages from mapreduce.{h,cc}\n" +
"b. Print VLOG(1) and lower messages from file.{h,cc}\n" +
"c. Print VLOG(3) and lower messages from files prefixed with \"gfs\"\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. stray spaces?

Remove the "new" SqlQueryScheduler, which was created to enable stage
retries, but was never rolled out and is no longer important because of
Presto-on-Spark. Rename LegacySqlQueryScheduler to SqlQueryScheduler.
@steveburnett
Copy link
Contributor

I checked the documentation and none of these properties appear anywhere in presto-docs other than the 0.232 and 0.239 release notes, so no doc updates needed.

Copy link
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

LGTM

@rschlussel rschlussel merged commit 1e1963c into prestodb:master Feb 27, 2024
56 checks passed
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate LegacySqlQueryScheduler and SqlQueryScheduler
5 participants