Skip to content

Add support for the Trino client spooling protocol#496

Closed
mdesmet wants to merge 4 commits into
trinodb:masterfrom
starburstdata:feat/spooled-protocol
Closed

Add support for the Trino client spooling protocol#496
mdesmet wants to merge 4 commits into
trinodb:masterfrom
starburstdata:feat/spooled-protocol

Conversation

@mdesmet
Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread trino/client.py Outdated
@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
Copy Markdown
Contributor

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
Comment thread trino/client.py Outdated
@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
Copy Markdown
Contributor

@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

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

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

@mosabua mosabua Dec 2, 2024

Choose a reason for hiding this comment

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

;-)

Comment thread tests/integration/test_dbapi_integration.py Outdated
Comment thread tests/integration/test_dbapi_integration.py Outdated
Comment thread trino/constants.py
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
Copy Markdown
Member

@mosabua mosabua Dec 2, 2024

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
Comment thread etc/spooling-manager.properties Outdated
Comment thread tests/development_server.py Outdated
Comment thread tests/integration/test_dbapi_integration.py Outdated
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from 992e25c to 5066858 Compare December 3, 2024 11:50
@mdesmet mdesmet requested a review from mosabua December 3, 2024 12:06
wendigo
wendigo reviewed Dec 3, 2024
View reviewed changes
Comment thread README.md Outdated
wendigo
wendigo reviewed Dec 3, 2024
View reviewed changes
Comment thread README.md Outdated
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from e01887b to 2a98915 Compare December 4, 2024 08:19
@wendigo
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
wendigo
wendigo reviewed Dec 5, 2024
View reviewed changes
Comment thread etc/spooling-manager.properties Outdated
wendigo
wendigo reviewed Dec 5, 2024
View reviewed changes
Comment thread tests/development_server.py
@mdesmet
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Comment thread 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
Copy Markdown
Contributor Author

@mdesmet mdesmet Dec 20, 2024

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.

Copy link
Copy Markdown
Contributor

@wendigo wendigo Dec 31, 2024

Choose a reason for hiding this comment

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

I've pushed small fix as the threading will be expensive to use. PTAL

Copy link
Copy Markdown
Contributor

@wendigo wendigo Dec 31, 2024

Choose a reason for hiding this comment

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

https://github.com/trinodb/trino-python-client/pull/508/commits (last commit)

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch 2 times, most recently from 5e38da4 to d1b43e4 Compare December 20, 2024 08:18
@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Dec 20, 2024

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

@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from d1b43e4 to a75433b Compare December 23, 2024 08:17
@mdesmet mdesmet force-pushed the feat/spooled-protocol branch from a75433b to f55789a Compare December 23, 2024 08:19
@mdesmet
Copy link
Copy Markdown
Contributor Author

mdesmet commented Dec 23, 2024

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

done

@mdesmet mdesmet closed this Jan 2, 2025
@hashhar
Copy link
Copy Markdown
Member

hashhar commented Jan 10, 2025

superseded by #509

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.

6 participants

@mdesmet @wendigo @hashhar @mosabua @aalbu @damian3031