Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for the Trino client spooling protocol #496

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for the Trino client spooling protocol #496

wants to merge 5 commits into from

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Nov 22, 2024 •
edited by mosabua
Loading

Description

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(*) Release notes are required, with the following suggested text:

* Add support for the Trino client spooling protocol

@mdesmet mdesmet marked this pull request as ready for review November 25, 2024 13:50
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch 2 times, most recently from 8ac3e6d to 2dc1909 Compare November 25, 2024 14:06
@mdesmet
Copy link
Contributor Author

mdesmet commented Nov 25, 2024

Currently tests are failing because of trinodb/trino#24226

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 2dc1909 to 6ec4713 Compare November 26, 2024 10:09
aalbu
aalbu reviewed Nov 26, 2024
View reviewed changes
trino/client.py Outdated Show resolved Hide resolved
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 6ec4713 to d7f4ee3 Compare November 27, 2024 04:40
@cla-bot cla-bot bot added the cla-signed label Nov 27, 2024
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from d7f4ee3 to 6dd0ee6 Compare November 27, 2024 08:44
@wendigo
Copy link

wendigo commented Nov 27, 2024

Can we skip this one failing test for now?

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 6dd0ee6 to f9f2825 Compare November 29, 2024 07:11
damian3031
damian3031 reviewed Nov 29, 2024
View reviewed changes
trino/client.py Outdated Show resolved Hide resolved
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch 8 times, most recently from 2bf6201 to 992e25c Compare December 1, 2024 15:28
wendigo
wendigo reviewed Dec 2, 2024
View reviewed changes
Copy link

@wendigo wendigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM except for decoding logic that needs to be improved

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
etc/config.properties Outdated Show resolved Hide resolved
etc/spooling-manager.properties Outdated Show resolved Hide resolved
etc/spooling-manager.properties Outdated Show resolved Hide resolved
trino/client.py Outdated Show resolved Hide resolved
trino/client.py Outdated Show resolved Hide resolved
trino/client.py Outdated Show resolved Hide resolved
trino/client.py Outdated Show resolved Hide resolved
tests/integration/test_dbapi_integration.py Outdated Show resolved Hide resolved
mosabua
mosabua requested changes Dec 2, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
etc/config.properties Show resolved Hide resolved
etc/config.properties Outdated Show resolved Hide resolved
etc/config.properties Outdated Show resolved Hide resolved
etc/spooling-manager.properties Outdated Show resolved Hide resolved
tests/development_server.py Outdated
.with_volume_mapping(str(root / "etc/catalog"), "/etc/trino/catalog")

# Enable spooling config
if TRINO_VERSION >= "466":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;-)

tests/integration/test_dbapi_integration.py Outdated Show resolved Hide resolved
tests/integration/test_dbapi_integration.py Outdated Show resolved Hide resolved
trino/constants.py
@@ -37,6 +37,7 @@
HEADER_CLIENT_TAGS = "X-Trino-Client-Tags"
HEADER_EXTRA_CREDENTIAL = "X-Trino-Extra-Credential"
HEADER_TIMEZONE = "X-Trino-Time-Zone"
HEADER_ENCODING = "X-Trino-Query-Data-Encoding"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should document that somewhere @wendigo .. dev guide in trino docs maybe?

aalbu
aalbu reviewed Dec 3, 2024
View reviewed changes
etc/spooling-manager.properties Outdated Show resolved Hide resolved
tests/development_server.py Outdated Show resolved Hide resolved
tests/integration/test_dbapi_integration.py Outdated Show resolved Hide resolved
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 992e25c to 5066858 Compare December 3, 2024 11:50
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 5066858 to e01887b Compare December 3, 2024 11:57
@mdesmet mdesmet requested review from wendigo, aalbu and mosabua December 3, 2024 12:05
wendigo
wendigo reviewed Dec 3, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
wendigo
wendigo reviewed Dec 3, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from e01887b to 2a98915 Compare December 4, 2024 08:19
@wendigo
Copy link

wendigo commented Dec 4, 2024

@mdesmet this is a good direction, the only comment I have is that the spooled segment data is resolved eagerly, I think that data fetch should happen in the SegmentDecoder when the spooled segment is iterated over, wdyt? (the same applies for ack)

@mdesmet
Copy link
Contributor Author

mdesmet commented Dec 5, 2024

@mdesmet this is a good direction, the only comment I have is that the spooled segment data is resolved eagerly, I think that data fetch should happen in the SegmentDecoder when the spooled segment is iterated over, wdyt? (the same applies for ack)

Actually it is not eagerly fetched. with @abstractproperty it is only fetched at the moment the .data property is used (in the SegmentDecoder decode method.

Following section in SegmentIterator manages both the acknowledgment and loading the next segment. So it actually works as you specify above.

try:
    self._current_segment = segment = next(self._segments)
    self._rows = iter(self._decoder.decode(segment))
except StopIteration:
    self._finished = True

@wendigo
Copy link

wendigo commented Dec 5, 2024

LGTM

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 2a98915 to 75a9e43 Compare December 5, 2024 07:36
@mdesmet
Copy link
Contributor Author

mdesmet commented Dec 5, 2024

Only remaining issue is making the acknowledge executed in a background thread.

@mdesmet mdesmet requested a review from wendigo December 8, 2024 04:38
@mosabua mosabua changed the title Implement spooled protocol Add support for the spooling protocol Dec 9, 2024
@mosabua mosabua changed the title Add support for the spooling protocol Add support for the Trino client spooling protocol Dec 9, 2024
@mosabua mosabua mentioned this pull request Dec 9, 2024
Open
@wendigo
Copy link

wendigo commented Dec 19, 2024

@mdesmet please rebase, how is the async ack going?

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 75a9e43 to 6431e2f Compare December 20, 2024 03:32
mdesmet
mdesmet commented Dec 20, 2024
View reviewed changes
trino/client.py
Comment on lines +1112 to +1123
def acknowledge_request():
try:
http_response = self._send_spooling_request(self.ack_uri, timeout=2)
if not http_response.ok:
self._request.raise_response_error(http_response)
except Exception as e:
logger.error(f"Failed to acknowledge spooling request for segment {self}: {e}")
# Start the acknowledgment in a background thread
thread = threading.Thread(target=acknowledge_request, daemon=True)
thread.start()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wendigo : apparently I didn't push my latest changes. Here is the async impl.

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 6431e2f to 5e38da4 Compare December 20, 2024 03:44
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 5e38da4 to d1b43e4 Compare December 20, 2024 08:18
@wendigo
Copy link

wendigo commented Dec 20, 2024

@mdesmet you can run tests on 467 and drop the last commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar Awaiting requested review from hashhar

@damian3031 damian3031 Awaiting requested review from damian3031

@aalbu aalbu Awaiting requested review from aalbu

@mosabua mosabua Awaiting requested review from mosabua

@wendigo wendigo Awaiting requested review from wendigo

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mdesmet @wendigo @mosabua @aalbu @damian3031