yihua commented on code in PR #11943:
URL: https://github.com/apache/hudi/pull/11943#discussion_r1805495002
##########
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());
+ }
+
+ public String getRecordMergerStrategy() {
+ return getString(RECORD_MERGER_STRATEGY);
+ }
+
+ public RecordMergeMode getRecordMergeMode() {
+ return RecordMergeMode.getValue(getString(RECORD_MERGE_MODE));
}
public HoodieRecordMerger getRecordMerger() {
- List<String> mergers =
StringUtils.split(getStringOrDefault(RECORD_MERGER_IMPLS), ",").stream()
- .map(String::trim)
- .distinct()
- .collect(Collectors.toList());
- String recordMergerStrategy = getString(RECORD_MERGER_STRATEGY);
- return HoodieRecordUtils.createRecordMerger(getString(BASE_PATH),
engineType, mergers, recordMergerStrategy);
+ return HoodieRecordUtils.createRecordMerger(getString(BASE_PATH),
engineType, getString(RECORD_MERGER_IMPLS), getString(RECORD_MERGER_STRATEGY));
Review Comment:
nit: this line is long. Better to break it into two lines.
##########
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());
+ }
+
+ public String getRecordMergerStrategy() {
+ return getString(RECORD_MERGER_STRATEGY);
+ }
+
+ public RecordMergeMode getRecordMergeMode() {
+ return RecordMergeMode.getValue(getString(RECORD_MERGE_MODE));
}
public HoodieRecordMerger getRecordMerger() {
- List<String> mergers =
StringUtils.split(getStringOrDefault(RECORD_MERGER_IMPLS), ",").stream()
- .map(String::trim)
- .distinct()
- .collect(Collectors.toList());
- String recordMergerStrategy = getString(RECORD_MERGER_STRATEGY);
- return HoodieRecordUtils.createRecordMerger(getString(BASE_PATH),
engineType, mergers, recordMergerStrategy);
+ return HoodieRecordUtils.createRecordMerger(getString(BASE_PATH),
engineType, getString(RECORD_MERGER_IMPLS), getString(RECORD_MERGER_STRATEGY));
Review Comment:
Is this API used by Avro merger or custom merge mode only?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1747,7 +1744,7 @@ public int getAsyncClusterMaxCommits() {
}
public String getPayloadClass() {
- return getString(HoodiePayloadConfig.PAYLOAD_CLASS_NAME);
+ return RecordPayloadType.getPayloadClassName(this);
Review Comment:
Have you checked if all existing logic of getting the payload class go
through this method? Since this logic now returns the default payload class
from the method instead of the config default value, we need to make sure any
logic using `getString(HoodiePayloadConfig.PAYLOAD_CLASS_NAME)` should migrate
to this API too, to avoid possible null or empty payload class name.
##########
hudi-common/src/main/java/org/apache/hudi/common/model/RecordPayloadType.java:
##########
@@ -98,12 +97,10 @@ public static String getPayloadClassName(HoodieConfig
config) {
String payloadClassName;
if (config.contains(PAYLOAD_CLASS_NAME)) {
payloadClassName = config.getString(PAYLOAD_CLASS_NAME);
- } else if (config.contains(PAYLOAD_TYPE)) {
- payloadClassName =
RecordPayloadType.valueOf(config.getString(PAYLOAD_TYPE)).getClassName();
} else if (config.contains("hoodie.datasource.write.payload.class")) {
payloadClassName =
config.getString("hoodie.datasource.write.payload.class");
} else {
- payloadClassName =
RecordPayloadType.valueOf(PAYLOAD_TYPE.defaultValue()).getClassName();
+ return DefaultHoodieRecordPayload.class.getName();
Review Comment:
Use the defined default legacy payload class variable here instead of
hardcoding the value.
##########
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:
Keep the old name using `.withAlternatives` if 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
