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

Metrics based custom scheduler plugin #24439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jp-sivaprasad
Copy link
Contributor

@jp-sivaprasad jp-sivaprasad commented Jan 27, 2025

Description

Metrics based custom scheduler plugin

Motivation and Context

Metrics based custom scheduler plugin. Schedules cluster with lowest number of queries (running + queued queries).
Support for implementation of third party custom schedulers to be used by Presto router.

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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 ==

Metrics based custom scheduler plugin Changes
* Add support for Metrics based custom scheduler plugin

@jp-sivaprasad jp-sivaprasad self-assigned this Jan 27, 2025
@jp-sivaprasad jp-sivaprasad requested a review from a team as a code owner January 27, 2025 12:48
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Jan 27, 2025
@prestodb-ci prestodb-ci requested review from a team, pramodsatya and nishithakbhaskaran and removed request for a team January 27, 2025 12:48
@jp-sivaprasad jp-sivaprasad force-pushed the presto-router-metrics-based-custom-scheduler-plugin branch from 4e47db2 to 83cab5c Compare January 27, 2025 13:04
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add the parent tag to make this module consistent with the others:

    <parent>
        <groupId>com.facebook.presto</groupId>
        <artifactId>presto-root</artifactId>
        <version>0.291-SNAPSHOT</version>
    </parent>

steveburnett
steveburnett previously approved these changes Jan 27, 2025
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

@jp-sivaprasad jp-sivaprasad force-pushed the presto-router-metrics-based-custom-scheduler-plugin branch from 83cab5c to 745c65b Compare January 28, 2025 22:01
@jp-sivaprasad jp-sivaprasad force-pushed the presto-router-metrics-based-custom-scheduler-plugin branch from 745c65b to f1fc17d Compare January 29, 2025 00:30
@tdcmeehan
Copy link
Contributor

@auden-woolfson can you PTAL?

@@ -0,0 +1 @@
router-scheduler.name=metricsBased
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to set this as a default value in the code, and leave this file empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The property name (router-scheduler.name) is added in the property file so that clients implementing a new scheduler plugin can just update the value to the name of the new scheduler. Also since the Metric based scheduler is just a sample implementation, it may not be good to set it as the default value in code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok... maybe it would make more sense to add a new optional property to the existing router-config.json file ("custom-scheduler-name" or something similar) instead of creating an entirely new file for this one property. This way would also allow for customers to use multiple different custom schedulers in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There can be more properties in future for new schedulers, like cpu/memory thresholds related to specific schedulers, which can use this property file

@steveburnett
Copy link
Contributor

Thanks for the release note!

New release note guidelines. Please remove the manual PR link in the following format from the release note entries for this PR.

:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

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.

6 participants