Support multi coordinator while serving query state info by abhiseksaikia · Pull Request #16163 · prestodb/presto

abhiseksaikia · 2021-05-26T19:30:55Z

Currently /v1/queryState/* endpoints serve query state info available within a particular coordinator. The change here is to enable these endpoints to retrieve cluster wide query state info.

Test plan - Verified though unit test and TpchQueryRunner

== RELEASE NOTES ==

General Changes
* Support multi coordinator for /v1/queryState/ endpoint

tdcmeehan · 2021-06-01T15:56:52Z

presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java

I think, given the asynchronous nature of the work in this method, it makes sense to make it completely async.

Look at examples in the codebase for:

Using @Suspended AsyncResponse asyncResponse to asynchronously return results in the endpoint.

Futures.transform / Futures.transformAsync to avoid blocking on the results of the futures.

It makes a lot of sense to make it non blocking and leverage asyncResponse here. I have made the changes.

tdcmeehan · 2021-06-01T15:59:41Z

presto-tests/src/test/java/com/facebook/presto/resourcemanager/BaseQueryResourceTest.java

Is this a pure refactoring or are there new tests introduced as part of it?

Also, I would prefer to just duplicate what's needed between the two tests. There's no need IMO to introduce an inheritance hierarchy.

Agree unless it's a shared tests that we want to run, lets not introduce inheritance in tests.

I think I did this to leverage same multi coordinator setup and teardown for both tests, but I agree we should avoid inheritance for the tests. I will make the change and may be leverage util/rules to share common stuff between the tests.

swapsmagic · 2021-06-01T18:15:18Z

presto-main/src/main/java/com/facebook/presto/server/QueryStateInfoResource.java

instead of pulling all info from RM, we could just use the other coordinator query info from RM and merge it with the existing query info. Not a must but a good to have optimization.

I think we can do this optimization. Only caveat is that we need to do back and forth GET-POST conversion while proxying request between coordinator and RM. I looked at the changes required for this and this seems like a relatively bigger one to add on top of the existing changes. I will create a separate PR for addressing this optimization.

swapsmagic · 2021-06-01T18:22:18Z

presto-tests/src/test/java/com/facebook/presto/resourcemanager/BaseQueryResourceTest.java

Agree unless it's a shared tests that we want to run, lets not introduce inheritance in tests.

tdcmeehan

Mostly good!

tdcmeehan · 2021-06-04T17:14:52Z

presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java

+    {
+        try {
+            Set<InternalNode> coordinators = internalNodeManager.getCoordinators();
+            List<ListenableFuture<List<QueryStateInfo>>> queryStateInfoFutureList = new ArrayList<>();


In general we use immutable data structures. Here I would either stream through the coordinators, perform the action and collect() it into an toImmutableList(), or use an ImmutableList.Builder

I have to get used to immutable data structures :) I made the changes to use ImmutableList.Builder

tdcmeehan · 2021-06-04T17:17:55Z

presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java

+    private static final JsonCodec<List<QueryStateInfo>> JSON_CODEC = JsonCodec.listJsonCodec(QueryStateInfo.class);
+    private final ResourceManagerClusterStateProvider clusterStateProvider;
+    private final InternalNodeManager internalNodeManager;
+    private ListeningExecutorService executor;


Make this final

tdcmeehan · 2021-06-04T17:18:52Z

presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java

+public class DistributedQueryInfoResource
+{
+    private static final Logger log = Logger.get(DistributedQueryInfoResource.class);
+    private static final JsonCodec<List<QueryStateInfo>> JSON_CODEC = JsonCodec.listJsonCodec(QueryStateInfo.class);


Typically this is wired in via Guice. See existing codecs defined in ResourceManagerModule.

Thanks for checking this! I missed this, it makes sense to use Guice here. I made the changes.

tdcmeehan

One question, otherwise LGTM

tdcmeehan · 2021-06-08T05:20:50Z

presto-main/src/main/java/com/facebook/presto/resourcemanager/DistributedQueryInfoResource.java

I think the logic here is simple enough, and not proxy-like enough, that it should just be implemented inline in this class. i.e. let's remove the changes to add this to the RM proxy, as it's not really proxying, it's just performing the request.

Makes sense since this class is meant to be used for proxy like stuff. I was using it like a client helper earlier. I made the change.

tdcmeehan · 2021-06-09T04:27:40Z

presto-tests/src/test/java/com/facebook/presto/resourcemanager/ResourceManagerTestHelper.java

There's technically nothing here that is specific to multi-coordinator setup or resource manager. Shall we rename this class to something like QueryExecutionClientUtil? I would also suggest making the HTTP client a parameter, that way we can just static import these methods in the tests that use them.

It was earlier named as a test helper to be used for tests under resource manager package, but it makes sense to move these to specific utils and we can use it in other tests as well if required. I made the changes.

Currently /v1/queryState/* endpoints serve query state info available within a particular coordinator. The change here is to enable these endpoints to retrieve cluster wide query state info.

abhiseksaikia force-pushed the asaikia_disagg_pqs branch 3 times, most recently from 8770903 to 3adb730 Compare May 26, 2021 20:45

abhiseksaikia changed the title ~~[WIP] Support multi coordinator while serving query state info~~ Support multi coordinator while serving query state info May 27, 2021

abhiseksaikia requested review from swapsmagic and tdcmeehan May 27, 2021 18:44

tdcmeehan requested changes Jun 1, 2021

View reviewed changes

swapsmagic reviewed Jun 1, 2021

View reviewed changes

abhiseksaikia force-pushed the asaikia_disagg_pqs branch 2 times, most recently from 1170aca to 286b755 Compare June 3, 2021 23:42

This was referenced Jun 3, 2021

[WIP] Support multi coordinator while serving resource group info #16203

Closed

Support multi coordinator while serving resource group info #16205

Merged

abhiseksaikia requested a review from tdcmeehan June 4, 2021 00:30

tdcmeehan requested changes Jun 4, 2021

View reviewed changes

abhiseksaikia force-pushed the asaikia_disagg_pqs branch 3 times, most recently from 0dd8aa8 to 1083051 Compare June 4, 2021 21:47

abhiseksaikia requested a review from tdcmeehan June 4, 2021 21:49

abhiseksaikia force-pushed the asaikia_disagg_pqs branch from 1083051 to 04a5a7b Compare June 7, 2021 21:38

tdcmeehan requested changes Jun 8, 2021

View reviewed changes

abhiseksaikia force-pushed the asaikia_disagg_pqs branch 5 times, most recently from 310f2aa to 948ad88 Compare June 9, 2021 02:56

tdcmeehan reviewed Jun 9, 2021

View reviewed changes

abhiseksaikia force-pushed the asaikia_disagg_pqs branch 3 times, most recently from 93a302f to 9ed08fd Compare June 10, 2021 15:31

tdcmeehan self-requested a review June 14, 2021 15:32

Support multi coordinator while serving query state info

0cd03a3

Currently /v1/queryState/* endpoints serve query state info available within a particular coordinator. The change here is to enable these endpoints to retrieve cluster wide query state info.

abhiseksaikia force-pushed the asaikia_disagg_pqs branch from 9ed08fd to 0cd03a3 Compare June 14, 2021 19:11

tdcmeehan approved these changes Jun 17, 2021

View reviewed changes

tdcmeehan merged commit 83fa968 into prestodb:master Jun 17, 2021

mayankgarg1990 mentioned this pull request Jun 18, 2021

Add release notes for 0.256 #16296

Merged

2 tasks

asjadsyed mentioned this pull request Aug 5, 2022

Support multi-coordinator while serving task info #18146

Merged

hackeryang mentioned this pull request Mar 29, 2023

[Design] Disaggregated Presto Coordinators #15453

Open

Conversation

abhiseksaikia commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhiseksaikia Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhiseksaikia Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhiseksaikia Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abhiseksaikia commented May 26, 2021 •

edited

Loading

abhiseksaikia Jun 1, 2021 •

edited

Loading

abhiseksaikia Jun 4, 2021 •

edited

Loading

abhiseksaikia Jun 8, 2021 •

edited

Loading