yihua commented on code in PR #11943:
URL: https://github.com/apache/hudi/pull/11943#discussion_r1799964658
##########
hudi-common/src/main/java/org/apache/hudi/common/util/HoodieRecordUtils.java:
##########
@@ -75,14 +75,14 @@ public static HoodieRecordMerger loadRecordMerger(String
mergerClass) {
* Instantiate a given class with a record merge.
*/
public static HoodieRecordMerger createRecordMerger(String basePath,
EngineType engineType,
- List<String>
mergerClassList, String recordMergerStrategy) {
- if (mergerClassList.isEmpty() ||
HoodieTableMetadata.isMetadataTable(basePath)) {
+ List<String>
mergerClassList, Option<String> recordMergerStrategy) {
+ if (mergerClassList.isEmpty() || recordMergerStrategy.isEmpty() ||
HoodieTableMetadata.isMetadataTable(basePath)) {
Review Comment:
How is this method different from other `getRecordMerger` methods? It would
be good to unify all similar logic to a single method.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/HoodieRecordUtils.java:
##########
@@ -75,14 +75,14 @@ public static HoodieRecordMerger loadRecordMerger(String
mergerClass) {
* Instantiate a given class with a record merge.
*/
public static HoodieRecordMerger createRecordMerger(String basePath,
EngineType engineType,
- List<String>
mergerClassList, String recordMergerStrategy) {
- if (mergerClassList.isEmpty() ||
HoodieTableMetadata.isMetadataTable(basePath)) {
+ List<String>
mergerClassList, Option<String> recordMergerStrategy) {
+ if (mergerClassList.isEmpty() || recordMergerStrategy.isEmpty() ||
HoodieTableMetadata.isMetadataTable(basePath)) {
Review Comment:
It would be better to use merge mode to determine the merger too, and this
can be in the `RecordMergeConfig` class.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/HoodieRecordUtils.java:
##########
@@ -75,14 +75,14 @@ public static HoodieRecordMerger loadRecordMerger(String
mergerClass) {
* Instantiate a given class with a record merge.
*/
public static HoodieRecordMerger createRecordMerger(String basePath,
EngineType engineType,
- List<String>
mergerClassList, String recordMergerStrategy) {
- if (mergerClassList.isEmpty() ||
HoodieTableMetadata.isMetadataTable(basePath)) {
+ List<String>
mergerClassList, Option<String> recordMergerStrategy) {
+ if (mergerClassList.isEmpty() || recordMergerStrategy.isEmpty() ||
HoodieTableMetadata.isMetadataTable(basePath)) {
Review Comment:
If the merge mode is `CUSTOM`, use `HoodieAvroRecordMerger`.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ConfigUtils.java:
##########
@@ -89,11 +92,17 @@ public static String getOrderingField(Properties
properties) {
return orderField;
}
+ public static RecordMergeMode getRecordMergeMode(Properties properties) {
+ return RecordMergeMode.getValue(getStringWithAltKeys(properties,
RECORD_MERGE_MODE, RECORD_MERGE_MODE.defaultValue().name()));
+ }
+
/**
* Get payload class.
*/
- public static String getPayloadClass(Properties properties) {
- return RecordPayloadType.getPayloadClassName(new HoodieConfig(properties));
+ public static String getAvroPayloadClass(Properties properties) {
+ HoodieConfig hoodieConfig = new HoodieConfig(properties);
+ return RecordPayloadType.getPayloadClassName(hoodieConfig).orElseGet(() ->
+
HoodieRecordPayload.getAvroPayloadForMergeMode(getRecordMergeMode(properties)));
}
Review Comment:
Let's fold all these into `RecordMergeConfig` class if possible, so the
relevant logic are in the same place.
--
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]
