Fix unnecessary cancel requests #136
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
@cla-bot check |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
@nineinchnick add a need unit test. I think it make sense |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
| qr.nextURI = "" | ||
| return nil | ||
|
|
||
| if qr.nextURI != "" { |
There was a problem hiding this comment.
Reverse this condition and do an early return to avoid indenting the remaining code.
| for i, v := range qr.coltype { | ||
| if i > len(dest)-1 { | ||
|
|
||
| row := qr.data[qr.rowindex] |
There was a problem hiding this comment.
Move any refactor not directly related to the issue this PR fixes to a separate commit.
| qr.rowindex++ | ||
|
|
||
| // Prefetch next set of rows |
There was a problem hiding this comment.
Useless comment - it explains the what, which is pretty obvious, but not the why, which is not. Why do we need to do this? If the user doesn't request any more data, we'll waste resources on one round-trip.
Can we do this only when cancelling the query?
There was a problem hiding this comment.
I have to rethink this PR. Now that I'm more familiar with the project, we can't approach it this way—it's a bit of a mess.
I'll work on it. Thanks!
| @@ -1973,3 +1973,198 @@ func TestForwardAuthorizationHeader(t *testing.T) { | |||
|
|
|||
| assert.NoError(t, db.Close()) | |||
| } | |||
|
|
|||
| func TestPagination(t *testing.T) { | |||
There was a problem hiding this comment.
Can you come up with a better name? I'm not sure what this test is for.
| } | ||
|
|
||
| if r.URL.Path == "/v1/statement/20210817_140827_00000_arvdv/1" { | ||
| if buf2 == nil { |
There was a problem hiding this comment.
Why are separate buffers needed here? Can we initialize them early, and remove this condition?
Overview
This PR fixes unnecessary delete requests when the query is already finished.
Explanation
This change addresses Issue #102
QueryRowfollowed by aScan, the Trino driver first calls theNextmethod and then the Close method. TheClosemethod triggers a delete request to cancel the query, even when the query has already completed.nextUriin theClosemethod instead of directly calling/v1/query/queryId, as suggested in the documentation and implemented by other drivers. However, to make this work, I needed to always updaterows.nextUriinside thefetchmethod, as it was previously retaining only the first URI set here. Despite these changes, the issue persisted.Nextmethod so that when all rows have been read, it fetches the next set of rows. This works because ourfetchmethod, along with the running Goroutines, followsnextUriuntil data is retrieved.QueryRow, this ensures that when the query reaches the finished state with no more data, nextUri is cleared, preventing an unnecessary request on Close().QueryContext, this is also fine because it fetches the next batch of data, ensuring that additional data (if available) is retrieved correctly.