-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feature(protocol): Add executionType to server info #26454
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR adds an executionType field to ServerInfo by defining an ExecutionType enum with JSON (de)serialization, extending both the C++ and Java models and updating the server endpoints to populate it, and adjusting existing tests to include executionType. Class diagram for updated ServerInfo and new ExecutionType (Java)classDiagram
class ServerInfo {
- NodeVersion nodeVersion
- String environment
- boolean coordinator
- boolean starting
- Optional<Duration> uptime
- Optional<ExecutionType> executionType
+ ServerInfo(...)
+ getNodeVersion()
+ getEnvironment()
+ isCoordinator()
+ isStarting()
+ getUptime()
+ getExecutionType()
}
class ExecutionType {
<<enum>>
JAVA
NATIVE
NATIVE_GPU
+ getValue()
+ fromValue(String value)
+ toString()
}
ServerInfo --> "Optional" ExecutionType
Class diagram for updated ServerInfo and new ExecutionType (C++)classDiagram
class ServerInfo {
+ NodeVersion nodeVersion
+ String environment
+ bool coordinator
+ bool starting
+ shared_ptr<Duration> uptime
+ shared_ptr<ExecutionType> executionType
}
class ExecutionType {
<<enum>>
JAVA
NATIVE
NATIVE_GPU
}
ServerInfo --> "shared_ptr" ExecutionType
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
High level question: because Presto doesn't support heterogenous deployments, couldn't we represent the underlying eval used as a more simple flag? And instead of displaying this per server, couldn't it be displayed as a more general property of the entire cluster, given that we won't ever be mixing the different types of eval within a single cluster? |
@tdcmeehan Ah I was operating under the assumption that it could be heterogeneous, could definitely simplify in that case. Maybe a field on the |
|
@johallar so I've been thinking that classifying Presto by the eval it uses doesn't fully encapsulate the fullness of what is different in that deployment. For example, when you run with a Presto cluster that uses C++ and Velox, it's not that you've swapped out the eval on the worker, there's a lot of other stuff that changes, like functions and their semantics, session properties, and optimizer rules. Some of this we now encapsulate in an SPI, and we now have a "Velox SPI" of sorts that properly wires Presto to the differences found in Velox's eval (for example, by delegating function evaluation in the coordinator into Velox). There's also lots of "isNativeExecution" property usages in the code, but I'm not a fan of that because it ties a single boolean flag to a singular thing--am I using Presto with Velox as the underlying evaluation engine using vectorized execution? I imagine there are some subtleties we'll eventually need to surface up to the frontend from the flavor we're building that uses GPUs. So that end, I'm wondering if we can do this in two parts. The first part is, let's just add a generic identifier to the UI, and it can be literally anything. We could indeed just pipe this out from The second part could be, how do I set all of these flag values depending on which flavor of eval I'm using? And then what we'd do is split out these config values depending on the flavor, and set the defaults depending on the flavor. And for this particular opaque identifier we'll introduce to |
|
@tdcmeehan I think that makes sense and step 1 would definitely satisfy the initial needs. Do you think a config flag would be appropriate for setting the generic identifier value? Then just to clarify, step two in this plan would default the flag in a smart way based on the flavor of eval, but would be more nuanced than just java/native/gpu? |
Description
With three variants of workers, there has been some desire to be able to determine a workers execution type easily. This adds an
executionTypefield to the/v1/infoendpoint in order to be able to determine that.The eventual goal would be to display this information in a worker list in the Presto UI for quick reference.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: