Hey Steven, That makes sense. We can be gentle since there is no real pressure, but it would be nice to get this cleaned up at some point. I want to ensure everyone is comfortable with the deprecation and removal process of table properties. I've updated the PR <https://github.com/apache/iceberg/pull/12315> slightly to only raise an exception when the property is used. I think would be a nice balance.
Kind regards, Fokko Op di 18 feb 2025 om 19:41 schreef Steven Wu <stevenz...@gmail.com>: > > When you migrate a table, I don't think everyone cleans up the old > properties > > That is exactly the scenario we are trying to guard against. Maybe old > stables are still using those deprecated properties. Hence we want to guard > against silent behavior change. > > > and then jobs start failing. > > Warning logs can be too gentle and missed by users. It is like the > deprecation warning in code API. > > Failing the table loading is like the compiling error for removed code > API. This is the explicit signal. The error msg can provide explicit > instruction on how to fix it (maybe with a link to doc/announcement). > > If we want a real gentle deprecation process for table properties, I guess > the full steps can be > > - 1.9 - warning logs with explicit msg that table loading shall fail > in a future release (like 1.10) > - 1.10 - fail the table loading > - 2.0 - remove all references to deprecated table properties > > > > > > > On Tue, Feb 18, 2025 at 9:57 AM Fokko Driesprong <fo...@apache.org> wrote: > >> I'm hesitant to fail the job. When you migrate a table, I don't think >> everyone cleans up the old properties, and then jobs start failing. >> >> Another approach is to warn until 2.0, and then remove them: >> https://github.com/apache/iceberg/pull/12315 >> >> LMKWYT >> >> Kind regards, >> Fokko >> >> Op di 18 feb 2025 om 16:25 schreef Robert Stupp <sn...@snazy.de>: >> >>> Also, as an idea, REST catalog services could return an error if those >>> deprecated properties are being set. Thoughts? >>> On 18.02.25 16:21, Robert Stupp wrote: >>> >>> Agree with both Steve's. Personally, I'm okay with removing those >>> properties - but using the proposed phased approach. >>> On 17.02.25 23:25, Steven Wu wrote: >>> >>> I have some concerns on the issue of silent behavior change that Steve >>> Zhang raised in the PR comment. E.g., users may set the location based on >>> the deprecated table property, With this change, it would silently switch >>> to a new location. This can potentially mess up orphan file cleanup etc. >>> >>> Maybe we should consider the more conservative two-step approach that >>> Steve mentioned (1) in the next release of 1.9, fail when those properties >>> were defined in the table (2) remove those properties' references maybe in >>> 2.0. >>> >>> On Mon, Feb 17, 2025 at 2:17 PM Kevin Liu <kevinjq...@apache.org> wrote: >>> >>>> +1 for removing. Thanks for taking up the cleanup duty! >>>> >>>> I looked up the usage for the property and its string value with github >>>> search, and confirmed that they are not used. >>>> >>>> Also, for reference, here are the previous related PRs: >>>> https://github.com/apache/iceberg/pull/3094 >>>> https://github.com/apache/iceberg/pull/2965 >>>> >>>> Best, >>>> Kevin Liu >>>> >>>> On Mon, Feb 17, 2025 at 2:06 PM Yufei Gu <flyrain...@gmail.com> wrote: >>>> >>>>> +1 to remove them. >>>>> Yufei >>>>> >>>>> >>>>> On Mon, Feb 17, 2025 at 1:26 PM Steve Zhang >>>>> <hongyue_zh...@apple.com.invalid> <hongyue_zh...@apple.com.invalid> >>>>> wrote: >>>>> >>>>>> Thanks Fokko for removing deprecated properties! >>>>>> >>>>>> Just want to highlight the worst case for tables with old >>>>>> configuration and not aware of this deprecation might experience silent >>>>>> behavior change. But considering this has been deprecated for past 3 >>>>>> years, >>>>>> here’s my +1. >>>>>> >>>>>> Thanks, >>>>>> Steve Zhang >>>>>> >>>>>> >>>>>> >>>>>> On Feb 17, 2025, at 2:18 AM, Fokko Driesprong <fo...@apache.org> >>>>>> wrote: >>>>>> >>>>>> Hi everyone, >>>>>> >>>>>> While reviewing the LocationProvider equivalent of PyIceberg, I >>>>>> noticed some old code in the Java codebase that I felt could be cleaned >>>>>> up. You >>>>>> can find the PR over here >>>>>> <https://github.com/apache/iceberg/pull/12174>. This one removes the >>>>>> deprecated properties: >>>>>> >>>>>> OBJECT_STORE_PATH = "write.object-storage.path"; >>>>>> WRITE_FOLDER_STORAGE_LOCATION = "write.folder-storage.path"; >>>>>> >>>>>> These have been deprecated since Iceberg 0.12, and would like to know >>>>>> if anyone has any concerns about removing these. >>>>>> >>>>>> Kind regards, >>>>>> Fokko >>>>>> >>>>>> >>>>>> -- >>> Robert Stupp >>> @snazy >>> >>> -- >>> Robert Stupp >>> @snazy >>> >>>
