Add support for Cursors through API Query Params #14110

vrajat · 2024-09-30T06:20:10Z

Cursor support will allow Pinot clients to consume query results in smaller chunks. This feature allows clients to work with lesser resources esp. memory. Application logic is simpler with cursors. For example an app UI paginates through results in a table or a graph. Cursor support has been implemented using APIs.

Design Doc

Implementation for #13185

API

POST /query/sql

A new broker API parameter has been added to trigger pagination.
The API accepts the following new optional query parameters:

getCursor(boolean):
numRows (int): The number of rows to return in the first page.

The response contains the following extra fields:

Field	Description
brokerHost	hostname of the processing broker
brokerPort	port of the processing broker
offset	starting offset of the result table slice in the cursor response
numRows	Number of rows in result table slice in the cursor response
cursorResultWriteTimeMs	Time taken to write the query response to ResponseStore
cursorFetchTimeMs	Time taken to read a slice of the result table from ResponseStore
submissionTime	Unix timestamp when the query was submitted
expirationTime	Unix timestamp when the response can be deleted from the ResponseStore
bytesWritten	number of bytes written to the response store when storing the result table

GET /resultStore/{requestId}/results

This is a broker API that can be used to iterate over the result set of a query submitted using the above API.
The API accepts the following query parameters:

offset (int) (required): The start offset of the page of results.
numRows (int) (optional): The number of rows in the page. By default it will use the default size.

GET /resultStore/{requestId}/

Returns the BrokerResponse metadata of the query.

GET /resultStore

Lists all the requestIds of all the query results available in the response store.

DELETE /resultStore/{requestId}/

Delete the results of a query.
The API accepts the following query parameters:

requestId (required)

SPI

The PR implements a FileSystem ResponseStore and a JSON ResponseSerde.

The feature provides two SPIs to extend the feature to support other implementations:

ResponseSerde: Serialize/Deserialize the response.
ResponseStore: Store responses in a storage system.
Both SPIs use Java SPI and the default ServiceLoader to find implementation of the SPIs. All implementation should be annotated with AutoService to help generate files for discovering the implementations.

Configuration

ResponseStore

Configuration	Default	Description
pinot.broker.cursor.response.store.type	file	The protocol to use for storage
pinot.broker.cursor.response.store.serde	json	The Serialization/Deserialization protocol to use for the result set

File Response Store

Configuration	Default	Description
pinot.broker.cursor.response.store.file.data.dir	/tmp/pinot/broker/response_store/data	Location where result files will be stored.
pinot.broker.cursor.response.store.file.data.dir	file:///tmp/pinot/broker/response_store/data	Location where temporary files will be created.
pinot.broker.cursor.response.store.file.extension	json	The file name extension

Miscellaneous

Configuration	Default	Description
pinot.broker.cursor.result.size	10000	The result size if numRows is not specified in the API call.
pinot.broker.cursor.response.store.expiration	1h	The time before a query result will be deleted.
controller.cluster.response.store.cleaner.frequencyPeriod	1h	The frequency of the periodic task that deletes expired query results
controller.cluster.response.store.cursor.cleaner.initialDelay	random	The initial delay before the first run of the periodic task.

tags: feature, multi-stage, release-notes

codecov-commenter · 2024-09-30T07:30:42Z

Codecov Report

Attention: Patch coverage is 3.73134% with 129 lines in your changes missing coverage. Please review.

Project coverage is 55.36%. Comparing base (59551e4) to head (fb03d62).
Report is 1199 commits behind head on master.

Files with missing lines	Patch %	Lines
...he/pinot/common/cursors/AbstractResponseStore.java	0.00%	53 Missing ⚠️
...t/common/response/broker/CursorResponseNative.java	0.00%	29 Missing ⚠️
...apache/pinot/spi/cursors/ResponseSerdeService.java	0.00%	22 Missing ⚠️
...apache/pinot/spi/cursors/ResponseStoreService.java	0.00%	22 Missing ⚠️
...e/pinot/common/utils/config/QueryOptionsUtils.java	0.00%	2 Missing ⚠️
...va/org/apache/pinot/spi/utils/CommonConstants.java	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (59551e4) and HEAD (fb03d62). Click for more details.

HEAD has 34 uploads less than BASE

Flag BASE (59551e4) HEAD (fb03d62)

integration 7 2

integration2 3 2

temurin 12 5

java-21 7 3

skip-bytebuffers-true 3 1

skip-bytebuffers-false 7 4

unittests 5 3

java-11 5 2

unittests2 3 0

integration1 2 0

custom-integration1 2 0

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14110      +/-   ##
============================================
- Coverage     61.75%   55.36%   -6.40%     
- Complexity      207      791     +584     
============================================
  Files          2436     2078     -358     
  Lines        133233   109585   -23648     
  Branches      20636    17341    -3295     
============================================
- Hits          82274    60668   -21606     
+ Misses        44911    44073     -838     
+ Partials       6048     4844    -1204

Flag	Coverage Δ
custom-integration1	`?`
integration	`0.00% <ø> (-0.01%)`	⬇️
integration1	`?`
integration2	`0.00% <ø> (ø)`
java-11	`55.32% <3.73%> (-6.39%)`	⬇️
java-21	`55.22% <3.73%> (-6.41%)`	⬇️
skip-bytebuffers-false	`55.35% <3.73%> (-6.40%)`	⬇️
skip-bytebuffers-true	`55.19% <3.73%> (+27.46%)`	⬆️
temurin	`55.36% <3.73%> (-6.40%)`	⬇️
unittests	`55.36% <3.73%> (-6.39%)`	⬇️
unittests1	`55.36% <3.73%> (+8.47%)`	⬆️
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Setup CursorRequestHandlerDelegate Checkpoint - compiles and sanity test works. Setup CursorRequestHandlerDelegate Checkpoint - compiles and sanity test works. add spi for cursors Checkpoint: testCursorWorkflow works. Checkpoint: most tests work. Fix checkstyle Register ResultStoreCleaner Redesign SPI. Make cursor integration test work Add Test for FsResultStore Add Test for Auth Remove unnecessary files. Checkpoint. Fix tests to use URLs Undo unnecessary changes.

Add comments and fix failures.

jackjlli · 2024-10-18T18:10:12Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+            CommonConstants.CursorConfigs.DEFAULT_QUERY_RESULT_SIZE);
+      }
+
+      if (numRows > CommonConstants.CursorConfigs.MAX_QUERY_RESULT_SIZE) {


Why do we want to introduce the MAX_QUERY_RESULT_SIZE here if there is no such restriction on the generic queries?

jackjlli · 2024-10-18T18:12:19Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

@@ -100,6 +103,9 @@
 public class PinotClientRequest {
  private static final Logger LOGGER = LoggerFactory.getLogger(PinotClientRequest.class);

+  @Inject
+  PinotConfiguration _brokerConf;


You don't have to inject the broker config here as it's already in the BaseBrokerRequestHandler class.

There is no getter right now to get the config. I can add that. However is that a better option than injecting the config ?

jackjlli · 2024-10-18T18:13:41Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/ResponseStoreResource.java

+  private static final Logger LOGGER = LoggerFactory.getLogger(ResponseStoreResource.class);
+
+  @Inject
+  private PinotConfiguration _brokerConf;


Same here, you might not need to inject the broker config here.

jackjlli · 2024-10-18T18:19:44Z

pinot-common/src/main/java/org/apache/pinot/common/cursors/AbstractResponseStore.java

+    }
+
+    response.setResultTable(
+        new ResultTable(resultTable.getDataSchema(), resultTable.getRows().subList(offset, sliceEnd)));


Does that mean every time a read request will fetch the whole result table into memory first and then pick the page from the memory?

This implementation does read the whole result table into memory. Does LinkedIn implementation support seeking within the result table ? Do you have suggestions on how can the interface be changed to also support seek ?

jackjlli · 2024-10-18T18:21:11Z

pinot-common/src/main/java/org/apache/pinot/common/response/CursorResponse.java

+   * Starts from 0.
+   * @return current offset.
+   */
+  int getOffset();


nit: put the methods for the same object together, i.e. swapping the setNumRows() and getOffset() methods

jackjlli · 2024-10-18T18:25:30Z

pinot-controller/src/main/java/org/apache/pinot/controller/cursors/ResponseStoreCleaner.java

+            x -> new InstanceInfo(x.getInstanceName(), x.getHostName(), Integer.parseInt(x.getPort()))));
+
+    try {
+      Map<String, List<CursorResponseNative>> brokerResponses = getAllQueryResults(brokers, Collections.emptyMap());


IIUC, it seems every controller will try to fetch all the results from all the brokers, is it kind of a waste of resources to do that? Could we distribute the cleanup workloads to controllers instead?

I assumed that only the lead controller will run the periodic tasks. Right now, the controller does not have any information about ResponseStores. Only the broker has information.

The major point to discuss - is it a requirement that a ResponseStore should be accessible from all nodes ? Then the default implementation of using the broker local filesystem has to be changed.

The current perioidc task and cleaner assumes that a only a specific broker knows about the responses it has stored.

vrajat force-pushed the cursors branch from 6b85e92 to 594a40c Compare September 30, 2024 06:29

vrajat force-pushed the cursors branch from 79a246b to 12a6250 Compare October 17, 2024 04:56

vrajat added 6 commits October 17, 2024 10:29

Checkstyle

9fd9389

Simplify test setup

7c69b23

Clean up of docs and apis

619b982

Checkstyle fixes

c3bc972

Cleanup diffs

d46760b

vrajat force-pushed the cursors branch from 12a6250 to d46760b Compare October 17, 2024 05:32

vrajat added 9 commits October 17, 2024 11:43

Cleanup diffs

644d099

Support MSE

f76c47d

Clena up if condition

602f2d0

Remove AbstractResponseSerde.java

2de91ec

Clean up ResponseSerde implementation

09a1d65

Clean up ResponseStore implementation

0d7315c

Add metrics

7ce9407

Add javadoc

7eb1d5e

Checkstyle

c8bbbb8

vrajat changed the title ~~Cursors~~ Add support for Cursors through API Query Params Oct 17, 2024

vrajat added feature multi-stage Related to the multi-stage query engine labels Oct 17, 2024

Update config names.

ebcfd5a

vrajat requested review from Jackie-Jiang, yashmayya and gortiz October 17, 2024 16:26

vrajat marked this pull request as ready for review October 17, 2024 16:27

vrajat added 3 commits October 18, 2024 10:05

Improve tests

8754e55

Add comments and fix failures.

Checkstyle

014df89

Merge branch 'master' into cursors

37e5c34

Add comments

fb03d62

jackjlli reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Cursors through API Query Params #14110

Add support for Cursors through API Query Params #14110

vrajat commented Sep 30, 2024 •

edited

Loading

codecov-commenter commented Sep 30, 2024 •

edited

Loading

jackjlli Oct 18, 2024

jackjlli Oct 18, 2024

vrajat Oct 19, 2024

jackjlli Oct 18, 2024

jackjlli Oct 18, 2024

vrajat Oct 19, 2024

jackjlli Oct 18, 2024

jackjlli Oct 18, 2024

vrajat Oct 19, 2024

Add support for Cursors through API Query Params #14110

Are you sure you want to change the base?

Add support for Cursors through API Query Params #14110

Conversation

vrajat commented Sep 30, 2024 • edited Loading

API

POST /query/sql

GET /resultStore/{requestId}/results

GET /resultStore/{requestId}/

GET /resultStore

DELETE /resultStore/{requestId}/

SPI

Configuration

ResponseStore

File Response Store

Miscellaneous

codecov-commenter commented Sep 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrajat commented Sep 30, 2024 •

edited

Loading

codecov-commenter commented Sep 30, 2024 •

edited

Loading