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 deprecated WRITE_METADATA_LOCATION and WRITE_FOLDER_STORAGE_LOCATION #12174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Feb 4, 2025

I was digging into this for PyIceberg, this has been deprecated since 0.12.0

…_LOCATION`

Has been deprecated since 0.12.0
Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

[Minor] Looks like we wanna drop OBJECT_STORE_PATH and WRITE_FOLDER_STORAGE_LOCATION rather than PR heading
WRITE_METADATA_LOCATION and WRITE_FOLDER_STORAGE_LOCATION

+1 for removing !

Comment on lines -140 to +137
dataLocation = properties.get(TableProperties.OBJECT_STORE_PATH);
if (dataLocation == null) {
dataLocation = properties.get(TableProperties.WRITE_FOLDER_STORAGE_LOCATION);
if (dataLocation == null) {
dataLocation = String.format("%s/data", tableLocation);
}
}
dataLocation = String.format("%s/data", tableLocation);
Copy link
Contributor

@dramaticlly dramaticlly Feb 6, 2025

Choose a reason for hiding this comment

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

I enjoy remove deprecated method or config but it's always a bit risky. From a iceberg user perspective, if I had a running iceberg data pipeline, I might not ever going to change/move away from deprecated one.

So in a worst case scenario for a given table with OBJECT_STORE_PATH or WRITE_FOLDER_STORAGE_LOCATION is set and its value is not equal to WRITE_DATA_LOCATION, shall we abort the write to force user action?

I think current logic is ignoring the old table properties and directly write into default, I think it might be reasonable but want to know if this is intended. Maybe we can raise the awareness on dev list about the change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants