yihua commented on code in PR #11943:
URL: https://github.com/apache/hudi/pull/11943#discussion_r1805478907
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -168,21 +167,21 @@ public class HoodieWriteConfig extends HoodieConfig {
public static final ConfigProperty<String> WRITE_PAYLOAD_CLASS_NAME =
ConfigProperty
.key("hoodie.datasource.write.payload.class")
- .defaultValue(DefaultHoodieRecordPayload.class.getName())
+ .noDefaultValue()
.markAdvanced()
+ .deprecatedAfter("1.0.0")
.withDocumentation("Payload class used. Override this, if you like to
roll your own merge logic, when upserting/inserting. "
+ "This will render any value set for PRECOMBINE_FIELD_OPT_VAL
in-effective");
- public static final ConfigProperty<String> WRITE_PAYLOAD_TYPE =
ConfigProperty
- .key("hoodie.datasource.write.payload.type")
- .defaultValue(RecordPayloadType.HOODIE_AVRO_DEFAULT.name())
- .markAdvanced()
+ public static final ConfigProperty<String> RECORD_MERGE_MODE = ConfigProperty
+ .key("hoodie.datasource.write.record.merge.mode")
Review Comment:
```suggestion
.key("hoodie.write.record.merge.mode")
```
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/BootstrapCommand.java:
##########
@@ -72,8 +72,14 @@ public String bootstrap(
help = "Class for Full bootstrap input provider") final String
fullBootstrapInputProvider,
@ShellOption(value = {"--schemaProviderClass"}, defaultValue = "",
help = "SchemaProvider to attach schemas to bootstrap source data")
final String schemaProviderClass,
- @ShellOption(value = {"--payloadClass"}, defaultValue =
"org.apache.hudi.common.model.OverwriteWithLatestAvroPayload",
- help = "Payload Class") final String payloadClass,
+ @ShellOption(value = {"--payloadClass"}, defaultValue = "",
+ help = "Payload Class (deprecated). Use merge-mode for overwrite or
event time merging.") final String payloadClass,
+ @ShellOption(value = {"--merge-mode", "--record-merge-mode"},
defaultValue = "",
+ help = "Merge mode to use. 'EVENT_TIME_ORDERING',
'OVERWRITE_WITH_LATEST', or 'CUSTOM' if you want to set a merge strategy")
final String recordMergeMode,
+ @ShellOption(value = {"--merger-strategy", "--record-merger-strategy"},
defaultValue = "",
Review Comment:
We can make all record merge configs to start with `--record-merge-`,
`hoodie.write.record.merge.`, ``hoodie.table.record.merge.`, e.g.,
`--record-merge-strategy`, `hoodie.write.record.merge.strategy`, to be
consistent.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -191,7 +190,7 @@ public class HoodieWriteConfig extends HoodieConfig {
public static final ConfigProperty<String> RECORD_MERGER_STRATEGY =
ConfigProperty
.key("hoodie.datasource.write.record.merger.strategy")
- .defaultValue(HoodieRecordMerger.DEFAULT_MERGER_STRATEGY_UUID)
+ .noDefaultValue()
Review Comment:
Same here on consistent config naming.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -844,11 +843,6 @@ public class HoodieWriteConfig extends HoodieConfig {
*/
@Deprecated
public static final String WRITE_PAYLOAD_CLASS =
WRITE_PAYLOAD_CLASS_NAME.key();
- /**
- * @deprecated Use {@link #WRITE_PAYLOAD_CLASS_NAME} and its methods instead
- */
- @Deprecated
- public static final String DEFAULT_WRITE_PAYLOAD_CLASS =
WRITE_PAYLOAD_CLASS_NAME.defaultValue();
Review Comment:
We can hardcode the legacy default value here.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1239,16 +1233,19 @@ public String getBasePath() {
}
public HoodieFileFormat getBaseFileFormat() {
- return HoodieFileFormat.valueOf(getStringOrDefault(BASE_FILE_FORMAT));
+ return
HoodieFileFormat.valueOf(getStringOrDefault(BASE_FILE_FORMAT).toUpperCase());
Review Comment:
nit: could we handle the handling of case sensitivity inside
`HoodieFileFormat`?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -168,21 +167,21 @@ public class HoodieWriteConfig extends HoodieConfig {
public static final ConfigProperty<String> WRITE_PAYLOAD_CLASS_NAME =
ConfigProperty
.key("hoodie.datasource.write.payload.class")
- .defaultValue(DefaultHoodieRecordPayload.class.getName())
+ .noDefaultValue()
.markAdvanced()
+ .deprecatedAfter("1.0.0")
.withDocumentation("Payload class used. Override this, if you like to
roll your own merge logic, when upserting/inserting. "
+ "This will render any value set for PRECOMBINE_FIELD_OPT_VAL
in-effective");
- public static final ConfigProperty<String> WRITE_PAYLOAD_TYPE =
ConfigProperty
- .key("hoodie.datasource.write.payload.type")
- .defaultValue(RecordPayloadType.HOODIE_AVRO_DEFAULT.name())
- .markAdvanced()
+ public static final ConfigProperty<String> RECORD_MERGE_MODE = ConfigProperty
+ .key("hoodie.datasource.write.record.merge.mode")
+ .defaultValue(RecordMergeMode.EVENT_TIME_ORDERING.name())
Review Comment:
I remember that this can be switched back to `OVERWRITE_WITH_LATEST`. Is
there still an issue? I'm OK with `EVENT_TIME_ORDERING`; trying to see if it
still strictly requires precombine/ordering field to be set.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -191,7 +190,7 @@ public class HoodieWriteConfig extends HoodieConfig {
public static final ConfigProperty<String> RECORD_MERGER_STRATEGY =
ConfigProperty
Review Comment:
```suggestion
public static final ConfigProperty<String> RECORD_MERGE_STRATEGY_ID =
ConfigProperty
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -168,21 +167,21 @@ public class HoodieWriteConfig extends HoodieConfig {
public static final ConfigProperty<String> WRITE_PAYLOAD_CLASS_NAME =
ConfigProperty
.key("hoodie.datasource.write.payload.class")
- .defaultValue(DefaultHoodieRecordPayload.class.getName())
+ .noDefaultValue()
.markAdvanced()
+ .deprecatedAfter("1.0.0")
.withDocumentation("Payload class used. Override this, if you like to
roll your own merge logic, when upserting/inserting. "
+ "This will render any value set for PRECOMBINE_FIELD_OPT_VAL
in-effective");
- public static final ConfigProperty<String> WRITE_PAYLOAD_TYPE =
ConfigProperty
- .key("hoodie.datasource.write.payload.type")
- .defaultValue(RecordPayloadType.HOODIE_AVRO_DEFAULT.name())
- .markAdvanced()
+ public static final ConfigProperty<String> RECORD_MERGE_MODE = ConfigProperty
+ .key("hoodie.datasource.write.record.merge.mode")
+ .defaultValue(RecordMergeMode.EVENT_TIME_ORDERING.name())
.sinceVersion("1.0.0")
- .withDocumentation(RecordPayloadType.class);
+ .withDocumentation(RecordMergeMode.class);
public static final ConfigProperty<String> RECORD_MERGER_IMPLS =
ConfigProperty
.key("hoodie.datasource.write.record.merger.impls")
Review Comment:
```suggestion
.key("hoodie.write.record.merge.custom.impl.classes")
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodiePayloadConfig.java:
##########
@@ -65,9 +65,6 @@ public class HoodiePayloadConfig extends HoodieConfig {
.withDocumentation("Table column/field name to order records that have
the same key, before "
+ "merging and writing to storage.");
- /** @deprecated Use {@link #PAYLOAD_CLASS_NAME} and its methods instead */
- @Deprecated
- public static final String DEFAULT_PAYLOAD_CLASS =
PAYLOAD_CLASS_NAME.defaultValue();
Review Comment:
Regarding this variable for backwards compatible read
(https://github.com/apache/hudi/pull/11943/files#r1805475755), could we hard
code it to be `DefaultHoodieRecordPayload.class.getName()`?
--
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]
