feat: Add support for Trino 470 #705

NickLarsenNZ · 2025-02-05T16:07:44Z

Description

Part of stackabletech/docker-images#999.

Caution

Blocked by stackabletech/operator-rs#959.

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

Give feedback

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
Options

Reviewer

Give feedback

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)
Options

Acceptance

Give feedback

Feature Tracker has been updated
Proper release label has been added
Roadmap has been updated
Options

It was missed in #704

rust/operator-binary/src/catalog/commons.rs

sbernauer

Implementation LGTM, many thanks!

It would be great if you could please add some docs for the user. Maybe below https://docs.stackable.tech/home/stable/trino/#_supported_versions? Or put it somewhere else and link to it there.
IMHO it should contain a quick explanation that Trino switches it's S3 implementation and link to trinodb/trino#24878.
Also mention that users can roll back to the legacy using this and that configOverrides, however that it will be removed eventually.

Thanks!

CHANGELOG.md

rust/operator-binary/src/catalog/commons.rs

sbernauer · 2025-02-07T07:54:46Z

rust/operator-binary/src/catalog/mod.rs

@@ -70,6 +73,7 @@ pub trait ToCatalogConfig {
        catalog_name: &str,
        catalog_namespace: Option<String>,
        client: &Client,
+        trino_version: u16,


In all operators (and all other places in this operator AFAIK) we call it product_version of type &str. For consistency reasons it would be great to have that here as well.
However, as Trino is special I like the u16, so mixed feelings here ^^
If we go with u16, we should do it in all places in the operator I'd say.

Just FYI that Starburst versions Trino with a trailing e (I guess enterprise), e.g. 467-e. If we want to support that in the future (there have been ideas), a &str would be more robust.
TLDR: I let you decide, but please make it consistent within trino-op

Just FYI that Starburst versions Trino with a trailing e (I guess enterprise), e.g. 467-e.

ROFL, I had done a semver impl just in case, then decided against it since I didn't see any current use cases 🤦.

See 9a695e32394339569b78b1a75ae8c86750d39ecc..72a55733928e79fe5c3dcfde99b6903fc2feb7aa#diff-44935ae74c

Will respond separately to the first paragraph.

In all operators (and all other places in this operator AFAIK) we call it product_version of type &str. For consistency reasons it would be great to have that here as well.
However, as Trino is special I like the u16, so mixed feelings here ^^
If we go with u16, we should do it in all places in the operator I'd say.

I feel like it makes sense to represent the Trino version as a number as that is what it appears to be.

In the case of Starburst, the operator behaviour of 467-e should be the same as 467. Of course, if that ends up being false and we support both, then we would need to refactor it.

I have updated the JVM module to be consistent: 2640426

More generally, I think we should try to encode the differences in versions rather than having specific supported versions in code. Of course there will be versions we haven't validated against, but also we don't list them as supported, so I feel that is ok.

If all of our products versions can be coerced into semver, that could help too. That way we can still leverage PartialEq and PartialOrd. That could at least keep things consistent across the operators.

But at some point, there will be inconsistency and I think that is ok too. They are all wildly different products with whatever versioning scheme their respective maintainers choose.

sbernauer · 2025-02-07T07:58:27Z

rust/operator-binary/src/catalog/commons.rs

+            // Trino 469 deprecates the Legacy S3 (via the Hadoop FS interface) implementation.
+            // Trino 470 completely removes Legacy S3 support.


Hmm, it's a bit tricky.
Can you please link to trinodb/trino#24878 in the comments?
According to it, it was deprecated in 470 and will be removed in the future.
No idea what happened before 470 that required this change ^^

While they only advertise the deprecation in 470, it was functionally deprecated in 469, hence this work to get around it.

I will link that issue you mentioned.

Now that we're jumping straight to 470, would you like me to remove the comment about 469?

I think that makes sense, thanks!
But I let you decide

On second thought, I think we should still make the code valid even if we aren't shipping 469. We discovered the change necessary since then.

See improved comment in: 661fead

rust/crd/src/lib.rs

Maleware · 2025-02-10T12:54:32Z

rust/operator-binary/src/config/jvm.rs

+        // Copied from:
+        // - https://trino.io/docs/455/installation/deployment.html#jvm-config
+        // - https://trino.io/docs/469/installation/deployment.html#jvm-config
+        "455" | "469" => Ok(vec![


Suggested change

"455" | "469" => Ok(vec![

"455" | "470" => Ok(vec![

This has been changed to support numeric ranges.
See: 2640426

Specifically:

trino-operator/rust/operator-binary/src/config/jvm.rs

Lines 127 to 129 in 2640426

// - https://trino.io/docs/455/installation/deployment.html#jvm-config

// - https://trino.io/docs/470/installation/deployment.html#jvm-config

455.. => Ok(vec![

rust/operator-binary/src/config/jvm.rs

rust/operator-binary/src/controller.rs

tests/test-definition.yaml

… S3 connections Although it is technically possible to enable legacy S3 support in Trino 469, we have forced native s3 support since 469 so the warning is written that way.

…the end user

NickLarsenNZ · 2025-02-12T11:23:41Z

CHANGELOG.md


 ## Changed

+- BREAKING: Hard-code `s3.region=us-east-1` until it can be configured by the end user ([#705]).


Caution
We either need to merge with this in, or fix the CRD now to handle the s3 region.

NickLarsenNZ · 2025-02-12T11:24:08Z

docs/modules/trino/partials/supported-versions.adoc

@@ -2,5 +2,6 @@
 // This is a separate file, since it is used by both the direct Trino documentation, and the overarching
 // Stackable Platform documentation.

+- 469


Suggested change

- 469

- 470

NickLarsenNZ · 2025-02-12T11:24:32Z

rust/operator-binary/src/catalog/commons.rs

+            // Trino silently fails to load if `s3.region` is not set.
+            // We will hard-code it for now so that Trino can work with IONOS
+            // and Minio, but this will eventually come in via `S3Connection`
+            // once the CRD has the field.
+            tracing::warn!("s3.region is currently hard-coded to us-east-1");
+            catalog_config.add_property("s3.region", "us-east-1");


Caution
We either need to merge with this in, or fix the CRD now to handle the s3 region.

I would be in favor of adding the region to the S3Connection (via a decision). We need to think about defaults though

NickLarsenNZ added 5 commits February 5, 2025 17:02

test(integration): Add Trino 469, update latest version

6202d19

docs: Update trino version to 469

f4ca614

feat: Add support for Trino 469

d3de6b5

test(unit): Update trino version used in tests

bc1b03b

chore: regenerate Cargo.nix

94f3159

It was missed in #704

NickLarsenNZ added status/blocked scheduled-for/25.3.0 labels Feb 5, 2025

NickLarsenNZ self-assigned this Feb 5, 2025

chore: Update changelog

9a695e3

NickLarsenNZ requested a review from sbernauer February 5, 2025 16:11

NickLarsenNZ commented Feb 5, 2025

View reviewed changes

rust/operator-binary/src/catalog/commons.rs Outdated Show resolved Hide resolved

NickLarsenNZ mentioned this pull request Feb 5, 2025

chore(trino): Update container image to 470 ahead of Stackable Release 25.3.0 # stackabletech/docker-images#971

Open

NickLarsenNZ added 4 commits February 6, 2025 15:23

wip: native s3 initial cut

83ceb12

wip: more versiony stuff

72a5573

wip: parse Trino version as u16 version from String

30240b4

wip: remove comment and restore newline

5b6b45a

NickLarsenNZ removed the status/blocked label Feb 6, 2025

NickLarsenNZ mentioned this pull request Feb 6, 2025

feat(trino): Use 470 stackabletech/demos#149

Open

NickLarsenNZ added the status/blocker label Feb 6, 2025

sbernauer requested changes Feb 7, 2025

View reviewed changes

NickLarsenNZ added status/blocked and removed status/blocker labels Feb 7, 2025

sbernauer changed the title ~~feat: Add support for Trino 469~~ feat: Add support for Trino 470 Feb 10, 2025

Maleware requested changes Feb 10, 2025

View reviewed changes

NickLarsenNZ added 6 commits February 10, 2025 17:35

feat: Replace 469 with 470

46b26f1

test: Trino now requires TLS for S3

6983f3f

chore: Search and replace Trino productVersions

dbf9409

chore: Rename trino_version to product_version for consistency

d74ba65

refactor: Change product_version in the jvm mod to u16 for consistency

2640426

chore: Improve note about native s3 support

661fead

NickLarsenNZ added 5 commits February 12, 2025 10:58

chore: Update changelog

10c0bd8

chore: Update changelog

0953c0a

chore: Add warning about upcoming requirement for TLS requirement for…

4753151

… S3 connections Although it is technically possible to enable legacy S3 support in Trino 469, we have forced native s3 support since 469 so the warning is written that way.

refactor: Use ensure! instead of if not

f868c45

wip!: Hard-code s3.region to us-east-1 until is can be configured by …

81fe1be

…the end user

NickLarsenNZ commented Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for Trino 470 #705

feat: Add support for Trino 470 #705

NickLarsenNZ commented Feb 5, 2025 •

edited

Loading

Author

Reviewer

Acceptance

sbernauer left a comment

sbernauer Feb 7, 2025

NickLarsenNZ Feb 7, 2025

NickLarsenNZ Feb 12, 2025

sbernauer Feb 7, 2025

NickLarsenNZ Feb 7, 2025 •

edited

Loading

NickLarsenNZ Feb 10, 2025

sbernauer Feb 10, 2025

NickLarsenNZ Feb 12, 2025

NickLarsenNZ Feb 12, 2025

Maleware Feb 10, 2025

NickLarsenNZ Feb 12, 2025 •

edited

Loading

NickLarsenNZ Feb 12, 2025

NickLarsenNZ Feb 12, 2025

NickLarsenNZ Feb 12, 2025

sbernauer Feb 12, 2025

		// Trino 469 deprecates the Legacy S3 (via the Hadoop FS interface) implementation.
		// Trino 470 completely removes Legacy S3 support.

	// - https://trino.io/docs/455/installation/deployment.html#jvm-config
	// - https://trino.io/docs/470/installation/deployment.html#jvm-config
	455.. => Ok(vec![


		## Changed

		- BREAKING: Hard-code `s3.region=us-east-1` until it can be configured by the end user ([#705]).

feat: Add support for Trino 470 #705

Are you sure you want to change the base?

feat: Add support for Trino 470 #705

Conversation

NickLarsenNZ commented Feb 5, 2025 • edited Loading

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

sbernauer left a 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

NickLarsenNZ Feb 7, 2025 • edited Loading

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

NickLarsenNZ Feb 12, 2025 • edited Loading

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

NickLarsenNZ commented Feb 5, 2025 •

edited

Loading

NickLarsenNZ Feb 7, 2025 •

edited

Loading

NickLarsenNZ Feb 12, 2025 •

edited

Loading