Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION #2965

jackye1995 · 2021-08-13T03:47:32Z

@cobookman, @kbendick and @openinx brought up the issue during #2845 that the variable name WRITE_NEW_DATA_LOCATION is inconsistent with the actual value write.folder-storage.path. This PR deprecates the old variable name to use a name that is consistent with the actual value.

I looked into the original PR that introduced this which is #6, I think @rdblue suggested using the value write.folder-storage.path, but the author @mccheah did not update the variable name.

Please let me know if there is any concern about this deprecation.

kbendick

So I agree that the name is somewhat confusing, but now that we have added some consistency in the naming, does the multiple fallback resolution strategy (object storage path -> folder storage path -> default location) still make sense?

I believe that it does, to be somewhat consistent for users (as once a user has specified a path, they probably intend to keep it), but I’d be open to being swayed if somebody presented a strong argument.

With time, we should potentially consider deprecating allowing both to be set and keep them distinct (folder storage or object storage). But that’s definitely a discussion for the dev list and another day (if ever).

Overall the PR looks clean to me though 👍

I’m interested in other people’s thoughts, but in the absence of feedback this looks good to me.

kbendick · 2021-08-13T08:55:55Z

core/src/main/java/org/apache/iceberg/TableProperties.java

+  /**
+   * @deprecated will be removed in 0.14.0, use {@link #WRITE_FOLDER_STORAGE_LOCATION} instead
+   */
+  @Deprecated


+1 to the deprecation.

When the deprecation comes around, we should consider providing an action to migrate tables off of deprecated properties. In general, I think that would allow us to make smarter decisions about table property names as new needs surface. Something similar to the v2 table migration action. But that’s a long ways off.

I think migration is unnecessary. The actual value stored is always write.folder-storage.path, we are just changing the Java variable name. And as I noted, we will deprecate after 2 releases, so that people can have time to change their variable name.

Why will be removed in 0.14.0? Is there a convention to follow?

kbendick · 2021-08-13T08:58:08Z

core/src/test/java/org/apache/iceberg/TestLocationProvider.java

  public void testDefaultLocationProviderWithCustomDataLocation() {
    this.table.updateProperties()
-        .set(TableProperties.WRITE_NEW_DATA_LOCATION, "new_location")
+        .set(TableProperties.WRITE_FOLDER_STORAGE_LOCATION, "new_location")


Does it make sense to leave a test for the deprecated config value?

I’d love to even log something if a user sets it, but that can be considered later (if at all).

It is the same value, just using a different name to avoid confusion.

Yeah I see now. This won't affect any external folks unless they have some custom stuff that touches this config.

jackye1995 · 2021-08-13T17:03:38Z

close to force restart CI

flyrain · 2021-08-19T01:03:18Z

The PR looks good to me.

fallback strategy (object storage path -> folder storage path -> default location)

An open question: can we consolidate both write.folder-storage.path and write.object-storage.path? They are unnecessary complexity. Ironically, we have to introduce a new config to consolidate two olde ones, :-).

flyrain · 2021-08-19T01:04:01Z

cc @rdblue @aokolnychyi @RussellSpitzer

jackye1995 · 2021-08-19T19:53:03Z

@flyrain great point around this, I am fully for a consolidation, this PR is really to prepare for that. My thought is the following:

in Core: allow default data location for ObjectStorageLocationProvider #2845 I implemented the fallback strategy
in this PR, we first consolidate the variable name to be consistent
in a subsequent PR, we can deprecate write.object-storage.path and only have 1 overwrite config.

For 3, my thinking is to avoid the need to introduce yet another config key, although I understand the name write.folder-storage.path isn't really ideal. Ideally we can have write.data.path which parallels write.metadata.path, but I really don't want to introduce yet another variable. What's your thoughts on this?

RussellSpitzer

lgtm +1

Not sure about the deprecation message since i'm not sure we'll have the right version there when we actually release but otherwise seems good.

flyrain · 2021-08-19T20:51:43Z

in a subsequent PR, we can deprecate write.object-storage.path and only have 1 overwrite config.

For 3, my thinking is to avoid the need to introduce yet another config key, although I understand the name write.folder-storage.path isn't really ideal. Ideally we can have write.data.path which parallels write.metadata.path, but I really don't want to introduce yet another variable. What's your thoughts on this?

Nice plan! I like the name write.data.path, but write.folder-storage.path is fine since it is there for a while. We can bring it up in the next community sync-up. We can go with write.data.path if there aren't many users using write.folder-storage.path.

Also let us know if you don't have time for step 3, we are glad to help. Thanks @jackye1995!

rdblue · 2021-08-23T16:27:49Z

Thanks, @jackye1995!

jackye1995 · 2021-08-24T00:15:46Z

Thanks for approving! @flyrain great, let's bring this up in the next community meeting then. Meanwhile I still need to catch up with your discussion on dev list 😝

rdblue · 2021-09-08T23:49:32Z

+1 for deprecating write.folder-storage.path, but I'm not sure we'll be able to actually remove it, since that would break forward-compatibility when older writers write to a table. We should definitely update docs to use write.data.path, though.

flyrain · 2021-09-09T00:53:01Z

Cool. Let's deprecate both write.folder-storage.path and write.object-storage.path, and use a new name write.data.path instead.

kbendick · 2021-09-09T04:19:39Z

+1 to deprecating and using the new name. Kyle Bendickson

…

On Sep 8, 2021, at 5:53 PM, Yufei Gu ***@***.***> wrote: Cool. Let's deprecate both write.folder-storage.path and write.object-storage.path, and use a new name write.data.path instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2965 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLAXERB4SR3SGRX2E4TKADUBAAPRANCNFSM5CCY5L2A>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

github-actions bot added core spark labels Aug 13, 2021

Jack Ye added 2 commits August 12, 2021 20:49

Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION

41c9702

fix tests

c7b8afa

kbendick reviewed Aug 13, 2021

View reviewed changes

jackye1995 closed this Aug 13, 2021

jackye1995 reopened this Aug 13, 2021

kbendick approved these changes Aug 19, 2021

View reviewed changes

RussellSpitzer approved these changes Aug 19, 2021

View reviewed changes

rdblue approved these changes Aug 23, 2021

View reviewed changes

rdblue merged commit aef12c0 into apache:master Aug 23, 2021

flyrain mentioned this pull request Sep 9, 2021

Core: Consolidate write.folder-storage.path and write.object-storage.path to write.data.path #3094

Merged

kevinjqliu mentioned this pull request Feb 17, 2025

Remove deprecated OBJECT_STORE_PATH and WRITE_FOLDER_STORAGE_LOCATION #12174

Closed

Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION #2965

Core: rename WRITE_NEW_DATA_LOCATION to WRITE_FOLDER_STORAGE_LOCATION #2965

Uh oh!

Conversation

jackye1995 commented Aug 13, 2021

Uh oh!

kbendick left a comment

Choose a reason for hiding this comment

Uh oh!

kbendick Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

jackye1995 Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

flyrain Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

kbendick Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

jackye1995 Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

kbendick Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

jackye1995 commented Aug 13, 2021

Uh oh!

flyrain commented Aug 19, 2021

Uh oh!

flyrain commented Aug 19, 2021

Uh oh!

jackye1995 commented Aug 19, 2021

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue commented Aug 23, 2021

Uh oh!

jackye1995 commented Aug 24, 2021

Uh oh!

rdblue commented Sep 8, 2021

Uh oh!

flyrain commented Sep 9, 2021

Uh oh!

kbendick commented Sep 9, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flyrain commented Aug 19, 2021 •

edited

Loading