kbendick commented on a change in pull request #3504:
URL: https://github.com/apache/iceberg/pull/3504#discussion_r745387758
##########
File path:
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java
##########
@@ -364,7 +364,7 @@ private String operatorName(String suffix) {
// 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);
Review comment:
Do we need a corresponding change for the job level check? The one that
the variable `upsert` comes from?
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -242,6 +242,6 @@ private TableProperties() {
public static final String MERGE_CARDINALITY_CHECK_ENABLED =
"write.merge.cardinality-check.enabled";
public static final boolean MERGE_CARDINALITY_CHECK_ENABLED_DEFAULT = true;
- 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";
Review 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
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
