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.

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

Remove the Deprecated marker for S3StorageClassFilter #24979

Merged

Conversation

zhaner08
Copy link
Contributor

Description

hive.s3.storage-class-filter is still being used to filter out glacier objects when listing s3 objects. Remove the annotation to correct it

Additional context and related issues

Release notes

( X) This is not user-visible or is 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:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Feb 10, 2025
@zhaner08 zhaner08 requested a review from mosabua February 10, 2025 20:30
@github-actions github-actions bot added the hive Hive connector label Feb 10, 2025
@zhaner08 zhaner08 requested a review from pettyjamesm February 10, 2025 20:30
@pettyjamesm
Copy link
Member

LGTM. @mosabua - seems like this got accidentally marked as deprecated as part of #24880, but this particular config shouldn't be removed.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

This should NOT be merged. All hive.s3 properties are deprecated .. if this is used in the new file system then it needs to be moved over and renamed .. we are planning to completely remove support for S3 via the legacy Hive-based classes.

fyi @electrum to chime in with more help if needed.

@pettyjamesm
Copy link
Member

The issue is that this change is not implemented inside of the legacy S3 filesystem API (for reasons outlined in #21164) but rather directly in the Hive connector. So unlike the other S3 configs, this one in particular should persist beyond the deprecation. @electrum - feel free to weigh in if you disagree

@mosabua
Copy link
Member

mosabua commented Feb 10, 2025

Hm .. I will let @electrum discuss the details on how to approach this more with you, but from my perspective we should at minimum change the property name. And if this is in a class or package that we will remove .. we probably want to move it as well.

@mosabua
Copy link
Member

mosabua commented Feb 10, 2025

Maybe this is related and solves the issue - #24934

@wendigo
Copy link
Contributor

wendigo commented Feb 10, 2025

We should add this to a native fs

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

I missed this when reviewing the deprecation PR. This is indeed a Hive connector property and unrelated to the S3 file system, aside from the unfortunately confusing naming. We could rename it if that will reduce confusion, but that might also just cause churn for no real benefit.

@mosabua
Copy link
Member

mosabua commented Feb 10, 2025

Hmm.. but what about the linked PR then? #24934 .. isnt that the better solution / approach ..

@electrum
Copy link
Member

This property is specifically about Glacier, which is a separate cold-storage system, while the linked PR is for other normal S3 storage classes. How about we follow up and rename this property to hive.s3-glacier-filter?

@electrum electrum requested a review from mosabua February 10, 2025 21:51
@mosabua
Copy link
Member

mosabua commented Feb 10, 2025

This property is specifically about Glacier, which is a separate cold-storage system, while the linked PR is for other normal S3 storage classes. How about we follow up and rename this property to hive.s3-glacier-filter?

I think thats much better .. I hope it also limits usage of that property.. and we can get the other PR in as well for other use cases .. does that make sense to you as well @pettyjamesm and @zhaner08 ?

we should just use this PR to do the rename..

@electrum
Copy link
Member

Put another way, this property controls whether Trino will attempt to read S3 objects that are in Glacier. The linked PR allows writing objects to normal S3 using a different storage class.

@wendigo wendigo merged commit 7be6521 into trinodb:master Feb 11, 2025
57 checks passed
@github-actions github-actions bot added this to the 471 milestone Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector
Development

Successfully merging this pull request may close these issues.

5 participants