Skip to content

feat: add support for the client spooling protocol #793

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
Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

feat: add support for the client spooling protocol #793

wants to merge 30 commits into from

Conversation

razvan
Copy link
Member

@razvan razvan commented Sep 2, 2025 •
edited
Loading

Description

Part of github.com//issues/763
Depends on decision: https://github.com/stackabletech/decisions/issues/60

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

  • Changes are OpenShift compatible
  • CRD changes approved
  • CRD documentation for all fields, following the style guide.
  • Helm chart can be installed and deployed operator works
  • Integration tests passed (for non trivial changes)
  • Changes need to be "offline" compatible
  • Links to generated (nightly) docs added
  • Release note snippet added

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • (Integration-)Test cases added
  • Documentation added or updated. Follows the style guide.
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Feature Tracker has been updated
  • Proper release label has been added
  • Links to generated (nightly) docs added
  • Release note snippet added
  • Add type/deprecation label & add to the deprecation schedule
  • Add type/experimental label & add to the experimental features tracker

@razvan razvan self-assigned this Sep 2, 2025
@razvan razvan marked this pull request as ready for review September 4, 2025 16:09
@razvan razvan requested a review from a team September 4, 2025 16:09
sbernauer
sbernauer reviewed Sep 4, 2025
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Very nice feature! :)
I only looked at the CRD code as part of the decision

rust/operator-binary/src/crd/client_protocol.rs Outdated
pub enum SpoolingFileSystemConfig {
S3(S3SpoolingConfig),
}
// TODO: this is exactly the same as fault_tolerant_execution::S3ExchangeConfig
Copy link
Member

Choose a reason for hiding this comment

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

Good idea! Let's do this as soon as the decision passed (to avoid unnecessary work) :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've extracted the common Trino S3 config in a separate CRD module and added it as a field to FTE and Spooling CRDs.
Also I moved the Resolved* bits into config modules.

commit for crd::s3 and spooling
commit for fte

I hope this is what you meant.

rust/operator-binary/src/crd/client_protocol.rs Outdated
pub upload_part_size: Option<Quantity>,
}

pub struct ResolvedSpoolingProtocolConfig {
Copy link
Member

@sbernauer sbernauer Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally this isn't in the crd module, as this is controller logic. But let's see how the FTE and client spooling code sharing goes

@razvan razvan requested a review from sbernauer September 5, 2025 09:20
@razvan razvan mentioned this pull request Sep 5, 2025
Closed
20 tasks
@razvan razvan moved this to Development: In Review in Stackable Engineering Sep 9, 2025
@razvan
Copy link
Member Author

razvan commented Sep 9, 2025

🟢 CI https://testing.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/trino-operator-it-custom/53/

sbernauer
sbernauer requested changes Sep 11, 2025
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Thanks! Mostly small stuff. My two old comments still apply 🙈

sbernauer
sbernauer reviewed Sep 15, 2025
View reviewed changes
CHANGELOG.md Outdated
Comment on lines 11 to 14

### Removed

- Cluster wide FTE specific `configOverrides` has been removed in favor of the "classic" role/group overrides ([#793]).
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't released yet 😅 So we don't have a change between 25.7 and 25.11. Leaving it in might confuse users

But feel free to amend Support for fault-tolerant execution with this PR, as this PR is now part of that implementatiion

Copy link
Member Author

Choose a reason for hiding this comment

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

0ba2fed

@razvan razvan requested a review from sbernauer September 15, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbernauer sbernauer Awaiting requested review from sbernauer

Requested changes must be addressed to merge this pull request.

Assignees

@razvan razvan

Labels
None yet
Projects
Stackable Engineering
Status: Development: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@razvan @sbernauer