Use the Pinot Grpc Endpoint for Streaming Server Queries by elonazoulay · Pull Request #12332 · trinodb/trino

elonazoulay · 2022-05-10T23:04:25Z

Description

Add support for querying Pinot via the more efficient GRPC endpoint.

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Pinot connector

Related issues, pull requests, and links

Fixes [Pinot connector] Support gRPC connection to Pinot Broker #9296

Documentation

(x) Documentation issue #12944 is filed, and can be handled later.

Release notes

(x) Release notes entries required with the following suggested text:

# Pinot
* Add support for querying Pinot via the gRPC endpoint. ({issue}`9296 `)

hashhar

skimmed, leaving some comments about commit boundaries as well (seems too fine-grained)

I'll need to look at the new PageSource in more detail

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSegmentPageSource.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java

hashhar · 2022-05-11T05:21:29Z

We can re-order commits as

The below could/should be a single commit adding GRPC support (it was a bit difficult to see the "whole" GRPC streaming feature in current shape).

Add PinotConfig options for the grpc client
Add grpc hostmapping to the PinotHostMapper
Add PinotGrpcClient
Implement PinotDataFetcher

hashhar · 2022-05-11T05:22:38Z

For the GRPC streaming is the only retained memory the one consumed by the byte buffers used for retrieving the Grpc responses? Can we account for them in the PageSource? Anything else that would need to be accounted (DataTable?)?

elonazoulay · 2022-05-18T16:54:12Z

Reoardered the commits but had to keep Implement PinotDataFetcher separate as it depends on the Use maxPageSize in PinotSegmentPageSource commit. If anything I can squash those and move the 'Make PinotDataTableWithSize publicbefore theAdd Support for GRPC` commit.

re: memory size: the entire payload in bytes from the response is now added to the memory usage.

hashhar

Some comments about commits but looks good overall.

@Praveen2112 Can you please take a look at last commit to see if the PageSource implementation looks correct (in terms of memory accounting, isFinished and getReadTimeNanos).

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSegmentPageSource.java

xiangfu0

lgtm, thanks for adding this support @elonazoulay

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSegmentPageSource.java

martint · 2022-05-27T18:35:48Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

Should this be enabled by default?

Yep, less impact on the pinot servers and allows for larger result sets.

~~Pinot servers will have to enable the endpoint in their configuration though, by default it's not enabled. Can add a note in the documentation.~~

I've turned on the grpc flag on pinot by default recently. I think we can make this enabled after one or two versions.

martint · 2022-05-27T18:38:37Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcClient.java

The lambda should use k. Also, give it a more meaninful name. At a minimum, key, but maybe hostAndPort would make it more obvious and readable.

Who manages the lifecycle of these clients? Do they need to be torn down? Can they go into an "invalid" state (in which case, they'd need to be refreshed)?

Grpc clients are long lived and use internal threads to manage connections.
The default idle time is 30 minutes for the ManagedChannel to close idle connections.
I added life cycle management and a comment in the code.

DONE: Manage lifecycle - close grpc clients on shutdown

DONE: Separate configs and modules for grpc and legacy query clients

DONE: Remove unused configs (separate commit)

TODO: extract to separate commits

martint · 2022-05-27T18:43:41Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java

Given that the grpc setting is global to the server, Instead of passing both, it'd be better for these to implement a common interface and pass the appropriate one depending how the connector was initialized.

xiangfu0 · 2022-05-31T22:52:05Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/IdentityPinotHostMapper.java

+    public HostAndPort getServerGrpcHostAndPort(String serverHost, int grpcPort)
+    {
+        ServerInstance serverInstance = getServerInstance(serverHost);
+        return HostAndPort.fromParts(serverInstance.getHostname(), grpcPort);


Actually ServerInstance has an API to get grpcPort, so you don't need to pass it.
https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java#L93

This API can just be: public HostAndPort getServerGrpcHostAndPort(String serverHost)

In this case, you don't even need to have the config for grpcPort, just make this best effort try. If the grpc port is -1, you return null here, and the query will use netty query endpoint. Once server has grpc, then the query will use grpc query endpoint.

I tried this connecting to a real cluster and the server instance returns -1 for the grpc port, it did not work. Is it ok to keep it explicitly specified? lmk what you think @xiangfu0 .

You mean grpc is enabled but serverInstance gives grpc port -1? If that is the case, please keep this grpc port config as the backup and use the grpc port that > 0 if from the ServerInstance.
So the logic is best effort from ServerInstance if possible or use grpc port if not provided.

xiangfu0 · 2022-05-31T23:37:42Z

...trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientConfig.java

+public class PinotGrpcServerQueryClientConfig
+{
+    private int maxRowsPerSplitForSegmentQueries = Integer.MAX_VALUE - 1;
+    private int grpcPort = 8090;


I think we don't need this grpcPort, just get it directly if grpc is enabled.

xiangfu0 · 2022-06-06T06:40:33Z

@martint can you review this again?

martint · 2022-06-10T20:41:21Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotDataFetcher.java

+    {
+        Map<String, String> metadata = dataTable.getMetadata();
+        List<String> exceptions = new ArrayList<>();
+        metadata.forEach((k, v) -> {


Use more descriptive names for k and v. It's not clear what they represent.

Fixed, was from legacy code. Also thanks to @xiangfu0 for the commit!

martint · 2022-06-10T20:51:30Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotDataFetcher.java

+    interface Factory
+    {
+        PinotDataFetcher create(ConnectorSession session, String query, PinotSplit split);
+    }
+
+    interface PinotServerQueryClient
+    {
+        Iterator<PinotDataTableWithSize> queryPinot(ConnectorSession session, String query, String serverHost, List<String> segments);


Why do we need both a Factory and a PinotServerQueryClient interface? I'm not sure I understand the relationship between the two. Why do we need different implementations of the Factory, given that the query client is already abstracted to support both underlying protocols?

Make sense, I think we just need factory no need for PinotServerQueryClient

Sounds good! Updating.

Updated, apologies, should have removed that before.

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java

elonazoulay · 2022-06-16T13:03:10Z

TODO: extract to separate commits, lmk if that will make it easier to review

xiangfu0 · 2022-06-16T23:48:33Z

I feel it's good to keep the PR here for a full complete feature. It's also simpler for future reference.

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotDataTableWithSize.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java

...no-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotLegacyDataFetcher.java

xiangfu0

Thanks for putting this up!

martint · 2022-06-22T16:39:32Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java

This is no longer used

xiangfu0 · 2022-06-22T20:28:52Z

Thanks @martint @elonazoulay to get this PR in! 🥂

cla-bot bot added the cla-signed label May 10, 2022

elonazoulay requested review from ebyhr and hashhar May 10, 2022 23:04

ebyhr removed their request for review May 10, 2022 23:57

hashhar reviewed May 11, 2022

View reviewed changes

elonazoulay force-pushed the pinot-grpc branch from 85e6c57 to a1644fa Compare May 18, 2022 17:04

hashhar reviewed May 20, 2022

View reviewed changes

elonazoulay force-pushed the pinot-grpc branch from a1644fa to 9138b2c Compare May 23, 2022 19:19

elonazoulay requested a review from hashhar May 23, 2022 20:40

xiangfu0 approved these changes May 25, 2022

View reviewed changes

martint requested changes May 27, 2022

View reviewed changes

elonazoulay force-pushed the pinot-grpc branch 2 times, most recently from 68a14b4 to b068fbb Compare May 31, 2022 06:09

xiangfu0 reviewed May 31, 2022

View reviewed changes

xiangfu0 requested a review from martint June 4, 2022 02:59

martint reviewed Jun 10, 2022

View reviewed changes

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java Outdated Show resolved Hide resolved

elonazoulay added 3 commits June 16, 2022 05:55

Set pinotColumnTypes in constructor

1bebff1

Make methods with default access private in PinotSegmentPageSource

48b4a1b

Qualify estimatedNonNumericColumnSize in constructor

ddfd532

elonazoulay force-pushed the pinot-grpc branch from b068fbb to 7e06995 Compare June 16, 2022 12:55

xiangfu0 requested a review from martint June 16, 2022 18:45

martint reviewed Jun 16, 2022

View reviewed changes

elonazoulay force-pushed the pinot-grpc branch from 7e06995 to f231780 Compare June 20, 2022 09:15

elonazoulay requested review from martint and xiangfu0 June 20, 2022 21:21

xiangfu0 approved these changes Jun 21, 2022

View reviewed changes

martint reviewed Jun 22, 2022

View reviewed changes

Add GRPC Support

42c8fe9

elonazoulay force-pushed the pinot-grpc branch from f231780 to 42c8fe9 Compare June 22, 2022 18:51

martint merged commit ce0bef3 into trinodb:master Jun 22, 2022

github-actions bot added this to the 387 milestone Jun 22, 2022

colebow mentioned this pull request Jun 22, 2022

Add Trino 387 release notes #12881

Merged

martint mentioned this pull request Jun 22, 2022

Document Pinot gRPC settings #12944

Closed

Conversation

elonazoulay commented May 10, 2022 • edited by martint Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar commented May 11, 2022

Uh oh!

hashhar commented May 11, 2022

Uh oh!

elonazoulay commented May 18, 2022

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elonazoulay May 28, 2022 • 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!

elonazoulay May 28, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangfu0 commented Jun 6, 2022

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!

Uh oh!

elonazoulay commented Jun 16, 2022

Uh oh!

xiangfu0 commented Jun 16, 2022

Uh oh!

Uh oh!

elonazoulay commented May 10, 2022 •

edited by martint

Loading

elonazoulay May 28, 2022 •

edited

Loading

elonazoulay May 28, 2022 •

edited

Loading