-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Support multi-coordinator while serving task info #18146
Conversation
a9616e7
to
ff9118a
Compare
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/QueryStateInfoResource.java
Outdated
Show resolved
Hide resolved
ff9118a
to
186ed47
Compare
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedTaskInfoResource.java
Outdated
Show resolved
Hide resolved
LGTM % @swapsmagic feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good. Left some comments.
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedTaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/QueryStateInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
...tests/src/test/java/com/facebook/presto/resourcemanager/TestDistributedTaskInfoResource.java
Outdated
Show resolved
Hide resolved
...tests/src/test/java/com/facebook/presto/resourcemanager/TestDistributedTaskInfoResource.java
Show resolved
Hide resolved
...tests/src/test/java/com/facebook/presto/resourcemanager/TestDistributedTaskInfoResource.java
Show resolved
Hide resolved
...tests/src/test/java/com/facebook/presto/resourcemanager/TestDistributedTaskInfoResource.java
Outdated
Show resolved
Hide resolved
72c7b3c
to
d43d33c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedTaskInfoResource.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/utils/QueryExecutionClientUtil.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedTaskInfoResource.java
Outdated
Show resolved
Hide resolved
7eababb
to
07fe3e1
Compare
presto-main/src/main/java/com/facebook/presto/server/TaskInfoResource.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % couple of comments
835a949
to
857b8ea
Compare
857b8ea
to
63fb402
Compare
63fb402
to
d2d848b
Compare
Summary:
The
/v1/taskInfo/{taskId}
endpoint serves task info available within a particular coordinator.With multi-coordinator, if a different coordinator is running the query, the endpoint currently returns 404. This leads to a broken UI under multi-coordinator, so fixing this issue will help with debugging. This PR fixing this by enabling the endpoint to retrieve cluster-wide task info.
We do this by implementing a proxying endpoint
/v1/taskInfo/{taskId}
in the resource manager: if the query isn't available on the queried coordinator, the coordinator will proxy the request to the RM, which will proxy it to the right coordinator.We also prevent an infinite-loop situation where the RM and coordinator could end up handing the request back and forth in case the coordinator no longer has info about the query, which might happen if the query was recently finished. The RM avoids the situation by adding a query parameter to let the coordinator know not to proxy it back again.
This PR is very similar to #16163, which fixed this issue for
/v1/queryState/*
. DistributedTaskInfoResource is to TaskInfoResource as DistributedQueryInfoResource is to QueryStateInfoResource.Test plan:
I've added a unit test (based off of TestDistributedQueryInfoResource) which starts a query on one coordinator and verifies we can retrieve the taskInfo on the second coordinator.
Release Notes: