You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/04/06 21:06:40 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
kbendick opened a new pull request, #4519:
URL: https://github.com/apache/iceberg/pull/4519
In https://github.com/apache/iceberg/pull/4364, we fixed an issue that was causing data correctness bugs when using `upsert` mode when writing to v2 table's with Flink.
The fix for the issue did not resolve the issue in Flink 1.12: https://github.com/apache/iceberg/pull/4418
Because Flink 1.12 has been deprecated, it was decided that we should disable `upsert` mode in Flink 1.12 for the upcoming patch release, Iceberg 0.13.2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#discussion_r847783780
##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java:
##########
@@ -387,19 +387,13 @@ private String operatorName(String suffix) {
boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(),
UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT);
- // Validate the equality fields and partition fields if we enable the upsert mode.
+ // `upsert` mode is not allowed in Flink 1.12 due to correctness issues.
+ // See in https://github.com/apache/iceberg/pull/4364 for more information.
if (upsertMode) {
- Preconditions.checkState(!overwrite,
- "OVERWRITE mode shouldn't be enable when configuring to use UPSERT data stream.");
- Preconditions.checkState(!equalityFieldIds.isEmpty(),
- "Equality field columns shouldn't be empty when configuring to use UPSERT data stream.");
- if (!table.spec().isUnpartitioned()) {
- for (PartitionField partitionField : table.spec().fields()) {
- Preconditions.checkState(equalityFieldIds.contains(partitionField.sourceId()),
- "In UPSERT mode, partition field '%s' should be included in equality fields: '%s'",
- partitionField, equalityFieldColumns);
- }
- }
+ throw new UnsupportedOperationException(
+ "upsert mode is not supported in Flink 1.12 due to correctness issues. Please upgrade to Flink 1.13+ if " +
Review Comment:
Updated to `Upsert`. Will fix unit tests as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1100301495
@openinx I left a note on the original PR to backport the fix to Flink 1.12 of which tests are failing (and repushed the branch so you could see for yourself).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1095211042
> Is there any blocker that we cannot just fix this bug in flink 1.12 ? In my context, I think the approach to fix the bug in fink 1.12 is just porting the patch from flink1.13/1.14 to flink1.12, I usually just export it to a patch file and replace the v1.14 to v1.12 globally.
My understanding is that there is a bug in Flink 1.12 that prevents us from fixing this, but I'm not sure what that is. I think we'll need @kbendick to jump in and give the details.
I agree with you, if we can easily adapt the fix for 1.13 and 1.14, then we should just fix it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
openinx commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1092390516
> Because Flink 1.12 has been deprecated, it was decided that we should disable upsert mode in Flink 1.12 for the upcoming patch release, Iceberg 0.13.2.
Will it be better to remove the flink 1.12 in the next iceberg release 0.14.0 ? As iceberg 0.13.2 is a bugfix release for the 0.13.x, Clearing the support for flink 1.12 does not sounds reasonable for me. I would like to port the #4418 into the flink 1.12, and once we decided to remove this we just need to remove the whole flink/v1.12 directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#discussion_r870923596
##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java:
##########
@@ -375,8 +375,22 @@ private SingleOutputStreamOperator<WriteResult> appendWriter(DataStream<RowData>
boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(),
UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT);
- // Validate the equality fields and partition fields if we enable the upsert mode.
+ // `upsert` mode should not be used in Flink 1.12 due to correctness issues.
+ // As part of a patch release, apachee-iceberg-flink-runtime_1.12:0.13.2,
+ // it has been decided the best course of action would be to log a warning
+ // asking people to upgrade, as Flink 1.12 is deprecated from both upstream
+ // Flink as well as Iceberg as of Iceberg 0.14.0. But we allow the configuration
+ // given that it's a patch release and the change would be otherwise very breaking.
+ //
+ // See in https://github.com/apache/iceberg/pull/4364 for more information.
if (upsertMode) {
+ String deprecationNotice =
+ "This job is running in upsert mode. Upsert mode should not be used with Flink 1.12 due to correctness " +
+ "issues. Please upgrade to Flink 1.13+ if upsert mode is truly needed. If upsert mode is not needed, " +
+ "set the table property `write.upsert.enabled` to 'false' and don't use `upsert(true)` while " +
Review Comment:
Should we mention that Flink 1.12 won't be supported in the next major Iceberg release? Support is also dropped upstream entirely (they won't even cherry-pick to 1.13 now), so it might be worth mentioning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue closed pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
URL: https://github.com/apache/iceberg/pull/4519
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#discussion_r870922581
##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java:
##########
@@ -375,8 +375,22 @@ private SingleOutputStreamOperator<WriteResult> appendWriter(DataStream<RowData>
boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(),
UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT);
- // Validate the equality fields and partition fields if we enable the upsert mode.
+ // `upsert` mode should not be used in Flink 1.12 due to correctness issues.
+ // As part of a patch release, apachee-iceberg-flink-runtime_1.12:0.13.2,
+ // it has been decided the best course of action would be to log a warning
+ // asking people to upgrade, as Flink 1.12 is deprecated from both upstream
+ // Flink as well as Iceberg as of Iceberg 0.14.0. But we allow the configuration
+ // given that it's a patch release and the change would be otherwise very breaking.
+ //
+ // See in https://github.com/apache/iceberg/pull/4364 for more information.
if (upsertMode) {
+ String deprecationNotice =
+ "This job is running in upsert mode. Upsert mode should not be used with Flink 1.12 due to correctness " +
+ "issues. Please upgrade to Flink 1.13+ if upsert mode is truly needed. If upsert mode is not needed, " +
+ "set the table property `write.upsert.enabled` to 'false' and don't use `upsert(true)` while " +
Review Comment:
Ok. I kept the language similat to the message before, but happy to update it however.
I can see what you mean by `if truly needed`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#discussion_r846842598
##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java:
##########
@@ -387,19 +387,13 @@ private String operatorName(String suffix) {
boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(),
UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT);
- // Validate the equality fields and partition fields if we enable the upsert mode.
+ // `upsert` mode is not allowed in Flink 1.12 due to correctness issues.
+ // See in https://github.com/apache/iceberg/pull/4364 for more information.
if (upsertMode) {
- Preconditions.checkState(!overwrite,
- "OVERWRITE mode shouldn't be enable when configuring to use UPSERT data stream.");
- Preconditions.checkState(!equalityFieldIds.isEmpty(),
- "Equality field columns shouldn't be empty when configuring to use UPSERT data stream.");
- if (!table.spec().isUnpartitioned()) {
- for (PartitionField partitionField : table.spec().fields()) {
- Preconditions.checkState(equalityFieldIds.contains(partitionField.sourceId()),
- "In UPSERT mode, partition field '%s' should be included in equality fields: '%s'",
- partitionField, equalityFieldColumns);
- }
- }
+ throw new UnsupportedOperationException(
+ "upsert mode is not supported in Flink 1.12 due to correctness issues. Please upgrade to Flink 1.13+ if " +
Review Comment:
We typically use sentence case for error messages. Can you use either `Upsert` or `UPSERT`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1094370049
@openinx, I think this is serious enough that we should not keep writing data in the 0.13.2 release if we can't fix it. Since we can't fix it in Flink 1.12, I think the right option is to disable it and ask the user to upgrade. If you want to fix it for the release, that would be great. But since we will probably remove support for 1.12 in the next release anyway, I don't think it makes much sense to spend time on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1124477315
As Flink 1.12 has been removed form master, this should likely be cherry-picked directly onto the branch for Flink release-base-0.13 or generally only targeting 0.13.2 and any later patch releases in the 0.13.x series (which are not likely to happen).
cc @rdblue @nastra throwing an exception in Flink for 0.13.2 has been changed to logging an error.
Please let me know if you'd like anything changed =)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#discussion_r870951772
##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java:
##########
@@ -375,8 +375,22 @@ private SingleOutputStreamOperator<WriteResult> appendWriter(DataStream<RowData>
boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(),
UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT);
- // Validate the equality fields and partition fields if we enable the upsert mode.
+ // `upsert` mode should not be used in Flink 1.12 due to correctness issues.
+ // As part of a patch release, apachee-iceberg-flink-runtime_1.12:0.13.2,
+ // it has been decided the best course of action would be to log a warning
+ // asking people to upgrade, as Flink 1.12 is deprecated from both upstream
+ // Flink as well as Iceberg as of Iceberg 0.14.0. But we allow the configuration
+ // given that it's a patch release and the change would be otherwise very breaking.
+ //
+ // See in https://github.com/apache/iceberg/pull/4364 for more information.
if (upsertMode) {
+ String deprecationNotice =
+ "This job is running in upsert mode. Upsert mode should not be used with Flink 1.12 due to correctness " +
+ "issues. Please upgrade to Flink 1.13+ if upsert mode is truly needed. If upsert mode is not needed, " +
+ "set the table property `write.upsert.enabled` to 'false' and don't use `upsert(true)` while " +
Review Comment:
Movedd this to an inline log (vs the variable), as the 1.12 CI isn't running as it's not in master, but I got a checkstyle exception using a non-constant expression in a log statement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1124582828
> I changed the base of this to be 0.13.x.
> Will work on fixes.
Just kidding! Changing the target branch to `0.13.x` from `master` made the diff massive. I'll open a new PR and apply a patch from here to there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1124581152
I changed the base of this to be 0.13.x.
Will work on fixes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
openinx commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1094761649
> Since we can't fix it in Flink 1.12, I think the right option is to disable it and ask the user to upgrade.
Sorry, maybe I missed something. Is there any blocker that we cannot just fix this bug in flink 1.12 ? In my context, I think the approach to fix the bug in fink 1.12 is just porting the patch from flink1.13/1.14 to flink1.12, I usually just export it to a patch file and replace the `v1.14` to `v1.12` globally.
I can also understand that we are planing to remove the flink 1.12 in the next iceberg release 0.14.0. But in iceberg release 0.13.2, I don't think we should remove flink 1.12 support because it's a bugfix release. So for the release 0.13.2, the correct approach is fixing this bug or disabling the `UPSERT` write feature. For me, fixing this bug in flink 1.12 seems more reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4519: Flink - Disallow upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1100221967
> > Is there any blocker that we cannot just fix this bug in flink 1.12 ? In my context, I think the approach to fix the bug in fink 1.12 is just porting the patch from flink1.13/1.14 to flink1.12, I usually just export it to a patch file and replace the v1.14 to v1.12 globally.
>
> My understanding is that there is a bug in Flink 1.12 that prevents us from fixing this, but I'm not sure what that is. I think we'll need @kbendick to jump in and give the details.
>
> I agree with you, if we can easily adapt the fix for 1.13 and 1.14, then we should just fix it!
@rdblue is correct. The patch from https://github.com/apache/iceberg/pull/4364 didn't work with Flink 1.12. The problem still persists. https://github.com/apache/iceberg/pull/4418 did not pass all unit tests and I was not able to resolve them. So I agree with Ryan that it's probably better to disable this one feature instead of continue to allow users to get incorrect results.
If they're somehow ok with the results they're getting, they can remain on Iceberg 0.13.1.
I repushed the branch from https://github.com/apache/iceberg/pull/4418 that originally had the patch for 1.12 . Let me try using `DATE` instead of `TO_DATE` to see if that resolves the issue. Otherwise, I think it's best to continue to disable this one feature in 1.12, given that we'll be removing 1.12 in the next major release anyway.
But if we can't get it to work, I don't think we should block the patch release on fixing it given that technically Flink 1.12 is deprecated upstream anyway and we've removed support in Iceberg 0.14.0-SNAPSHOT too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1124535536
cc @rdblue @nastra the requested changes have been made =)
I also ran `./gradlew clean -DflinkVersions=1.12,1.13,1.14 build -x javadoc -x integrationTest` and it ran to completion successfully (keep in mind 1.12 isn't in master CI - though it will be in your cherry-pick branch).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#discussion_r870920405
##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java:
##########
@@ -375,8 +375,22 @@ private SingleOutputStreamOperator<WriteResult> appendWriter(DataStream<RowData>
boolean upsertMode = upsert || PropertyUtil.propertyAsBoolean(table.properties(),
UPSERT_ENABLED, UPSERT_ENABLED_DEFAULT);
- // Validate the equality fields and partition fields if we enable the upsert mode.
+ // `upsert` mode should not be used in Flink 1.12 due to correctness issues.
+ // As part of a patch release, apachee-iceberg-flink-runtime_1.12:0.13.2,
+ // it has been decided the best course of action would be to log a warning
+ // asking people to upgrade, as Flink 1.12 is deprecated from both upstream
+ // Flink as well as Iceberg as of Iceberg 0.14.0. But we allow the configuration
+ // given that it's a patch release and the change would be otherwise very breaking.
+ //
+ // See in https://github.com/apache/iceberg/pull/4364 for more information.
if (upsertMode) {
+ String deprecationNotice =
+ "This job is running in upsert mode. Upsert mode should not be used with Flink 1.12 due to correctness " +
+ "issues. Please upgrade to Flink 1.13+ if upsert mode is truly needed. If upsert mode is not needed, " +
+ "set the table property `write.upsert.enabled` to 'false' and don't use `upsert(true)` while " +
Review Comment:
I think including the part about "if truly needed" isn't necessary. I think people only use upsert if it is needed. It would simplify the message to remove that part of it.
Also, can we be more specific about the problem? For example, "Upsert mode should not be used with Flink 1.12 because it will write incorrect delete file metadata, which could prevent deletes from being correctly applied. Upgrading to Flink 1.13+ is recommended. To safely use Flink 1.12, set manifest metrics to counts only."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4519: Flink - Log warning when in table upsert mode for Flink 1.12 due to correctness issues
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4519:
URL: https://github.com/apache/iceberg/pull/4519#issuecomment-1125062111
I merged the warning PR, so I don't think there is any more to do here. I'll close this one. Thanks, @kbendick!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org