Flink: Rename upsert table property #3504
Conversation
|
@openinx @Reo-LEI @rdblue @jackye1995 @RussellSpitzer, what do you think of this change? If I am not mistaken, this table property hasn't been released yet. |
aokolnychyi
force-pushed
the
rename-upsert-property
branch
from
6ed0390 to
aa3cdf0
Compare
|
@aokolnychyi Yes, I check the 0.11.x and 0.12.x and this change hasn't been released yet, rename the property is LGTM. |
kbendick
left a comment
kbendick
left a comment
There was a problem hiding this comment.
Left some comments.
If we allow both configs for a bit, we should log a warning or even an error level log to alert people if they use the old config value and give them time to change.
| // Fallback to use upsert mode parsed from table properties if don't specify in job level. | ||
| boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(), | ||
| UPSERT_MODE_ENABLE, UPSERT_MODE_ENABLE_DEFAULT); | ||
| UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT); |
There was a problem hiding this comment.
Do we need a corresponding change for the job level check? The one that the variable upsert comes from?
There was a problem hiding this comment.
I think you mean it's also good to name those variables to enabled.
|
|
||
| public static final String UPSERT_MODE_ENABLE = "write.upsert.enable"; | ||
| public static final boolean UPSERT_MODE_ENABLE_DEFAULT = false; | ||
| public static final String UPSERT_ENABLED = "write.upsert.enabled"; |
There was a problem hiding this comment.
Given that this is a breaking change, should we allow both for at least one release?
Especially given that this could be set in a number of places such as:
- table properties
- at the job level
- anything at the job level could be set in the clusters config file
There was a problem hiding this comment.
We don't have any released version since the PR got merged: #2863. I think it's OK to rename it to enabled.
I think it's my mistake because I did not notice that all the iceberg switch are named as xxx.enabled before. Thanks for the fix before we have a release, so that we don't have to maintain this inconsistent name for at least one release.
There was a problem hiding this comment.
Agree, I think we can just rename it as it hasn't been released.
There was a problem hiding this comment.
Ah that's great. If it hasn't been released then no worries. Retroactive +1.
aokolnychyi
force-pushed
the
rename-upsert-property
branch
from
aa3cdf0 to
7d2267b
Compare
aokolnychyi
force-pushed
the
rename-upsert-property
branch
from
7d2267b to
d3e9a4a
Compare
|
@openinx, I'll leave this one to you. Let me know if anything else needs to be updated. |
There was a problem hiding this comment.
LGTM, thanks @aokolnychyi for the PR and thanks all (@kbendick , @Reo-LEI ) the reviewing !
4 participants
This PR renames the upsert table property for consistency with other table props.