danny0405 commented on code in PR #9590:
URL: https://github.com/apache/hudi/pull/9590#discussion_r1320932463
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -602,18 +635,26 @@ public String getPartitionFieldProp() {
* Read the payload class for HoodieRecords from the table properties.
*/
public String getBootstrapIndexClass() {
- // There could be tables written with payload class from com.uber.hoodie.
Need to transparently
- // change to org.apache.hudi
- return getStringOrDefault(BOOTSTRAP_INDEX_CLASS_NAME,
getDefaultBootstrapIndexClass(props));
+ if (!props.getBoolean(BOOTSTRAP_INDEX_ENABLE.key(),
BOOTSTRAP_INDEX_ENABLE.defaultValue())) {
+ return BootstrapIndexType.NO_OP.getClassName();
+ }
+ String bootstrapIndexClassName;
+ if (contains(BOOTSTRAP_INDEX_TYPE)) {
+ bootstrapIndexClassName =
BootstrapIndexType.valueOf(getString(BOOTSTRAP_INDEX_TYPE)).getClassName();
+ } else if (contains(BOOTSTRAP_INDEX_CLASS_NAME)) {
+ bootstrapIndexClassName = getString(BOOTSTRAP_INDEX_CLASS_NAME);
+ } else {
+ bootstrapIndexClassName =
BootstrapIndexType.valueOf(BOOTSTRAP_INDEX_TYPE.defaultValue()).getClassName();
Review Comment:
It's better we move this method into the enum class itself, so that there is
no similiar patched logic scattered everywhere.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -602,18 +635,26 @@ public String getPartitionFieldProp() {
* Read the payload class for HoodieRecords from the table properties.
*/
public String getBootstrapIndexClass() {
- // There could be tables written with payload class from com.uber.hoodie.
Need to transparently
- // change to org.apache.hudi
- return getStringOrDefault(BOOTSTRAP_INDEX_CLASS_NAME,
getDefaultBootstrapIndexClass(props));
+ if (!props.getBoolean(BOOTSTRAP_INDEX_ENABLE.key(),
BOOTSTRAP_INDEX_ENABLE.defaultValue())) {
+ return BootstrapIndexType.NO_OP.getClassName();
+ }
+ String bootstrapIndexClassName;
+ if (contains(BOOTSTRAP_INDEX_TYPE)) {
+ bootstrapIndexClassName =
BootstrapIndexType.valueOf(getString(BOOTSTRAP_INDEX_TYPE)).getClassName();
+ } else if (contains(BOOTSTRAP_INDEX_CLASS_NAME)) {
+ bootstrapIndexClassName = getString(BOOTSTRAP_INDEX_CLASS_NAME);
+ } else {
+ bootstrapIndexClassName =
BootstrapIndexType.valueOf(BOOTSTRAP_INDEX_TYPE.defaultValue()).getClassName();
+ }
+ return bootstrapIndexClassName;
}
public static String getDefaultBootstrapIndexClass(Properties props) {
HoodieConfig hoodieConfig = new HoodieConfig(props);
- String defaultClass = BOOTSTRAP_INDEX_CLASS_NAME.defaultValue();
if (!hoodieConfig.getBooleanOrDefault(BOOTSTRAP_INDEX_ENABLE)) {
- defaultClass = NO_OP_BOOTSTRAP_INDEX_CLASS;
+ return BootstrapIndexType.NO_OP.getClassName();
}
- return defaultClass;
+ return
BootstrapIndexType.valueOf(BOOTSTRAP_INDEX_TYPE.defaultValue()).getClassName();
Review Comment:
Move this into `BootstrapIndexType`
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ConfigUtils.java:
##########
@@ -78,7 +79,9 @@ public static String getOrderingField(Properties properties) {
*/
public static String getPayloadClass(Properties properties) {
String payloadClass = null;
- if (properties.containsKey(HoodieTableConfig.PAYLOAD_CLASS_NAME.key())) {
+ if (properties.containsKey(HoodieTableConfig.PAYLOAD_TYPE.key())) {
+ payloadClass =
RecordPayloadType.valueOf(properties.getProperty(HoodieTableConfig.PAYLOAD_TYPE.key())).getClassName();
Review Comment:
Not sure why we need this method separately, can we move it altogether into
`RecordPayloadType`.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -447,7 +450,14 @@ public void validateTableProperties(Properties properties)
{
// Meta fields can be disabled only when either {@code
SimpleKeyGenerator}, {@code ComplexKeyGenerator}, {@code
NonpartitionedKeyGenerator} is used
if (!getTableConfig().populateMetaFields()) {
- String keyGenClass =
properties.getProperty(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME.key(),
"org.apache.hudi.keygen.SimpleKeyGenerator");
+ String keyGenClass;
+ if (properties.containsKey(HoodieTableConfig.KEY_GENERATOR_TYPE.key())) {
+ keyGenClass =
KeyGeneratorType.valueOf(properties.getProperty(HoodieTableConfig.KEY_GENERATOR_TYPE.key())).getClassName();
+ } else if
(properties.containsKey(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME.key())) {
+ keyGenClass =
properties.getProperty(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME.key());
+ } else {
+ keyGenClass = "org.apache.hudi.keygen.SimpleKeyGenerator";
Review Comment:
Move this into `KeyGeneratorType`
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -697,7 +738,13 @@ public HoodieCDCSupplementalLoggingMode
cdcSupplementalLoggingMode() {
}
public String getKeyGeneratorClassName() {
- return getString(KEY_GENERATOR_CLASS_NAME);
+ String keyGeneratorClassName = null;
+ if (contains(KEY_GENERATOR_TYPE)) {
+ keyGeneratorClassName =
KeyGeneratorType.valueOf(getString(KEY_GENERATOR_TYPE)).getClassName();
+ } else if (contains(KEY_GENERATOR_CLASS_NAME)) {
+ keyGeneratorClassName = getString(KEY_GENERATOR_CLASS_NAME);
+ }
Review Comment:
Move this into `KeyGeneratorType`
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -540,10 +566,17 @@ public void setTableVersion(HoodieTableVersion
tableVersion) {
* Read the payload class for HoodieRecords from the table properties.
*/
public String getPayloadClass() {
- // There could be tables written with payload class from com.uber.hoodie.
Need to transparently
- // change to org.apache.hudi
- return getStringOrDefault(PAYLOAD_CLASS_NAME).replace("com.uber.hoodie",
- "org.apache.hudi");
+ String payloadClassName;
+ if (contains(PAYLOAD_TYPE)) {
+ payloadClassName =
RecordPayloadType.valueOf(getString(PAYLOAD_TYPE)).getClassName();
+ } else if (contains(PAYLOAD_CLASS_NAME)) {
+ payloadClassName = getString(PAYLOAD_CLASS_NAME);
+ } else {
+ payloadClassName =
RecordPayloadType.valueOf(PAYLOAD_TYPE.defaultValue()).getClassName();
+ }
Review Comment:
It's better we move this method into the enum class itself, so that there is
no similiar patched logic scattered everywhere.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1243,7 +1250,11 @@ public String getPreCombineField() {
}
public String getWritePayloadClass() {
- return getString(WRITE_PAYLOAD_CLASS_NAME);
+ RecordPayloadType payloadType =
RecordPayloadType.valueOf(getString(WRITE_PAYLOAD_TYPE));
+ if (payloadType.equals(RecordPayloadType.CUSTOM)) {
+ return getString(WRITE_PAYLOAD_CLASS_NAME);
Review Comment:
How about the user does not set up the payload type explicitly, is there any
inference for that?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieWriterUtils.scala:
##########
@@ -230,7 +234,11 @@ object HoodieWriterUtils {
val diffConfigs = StringBuilder.newBuilder
if (null != tableConfig) {
- val tableConfigKeyGen =
tableConfig.getString(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME)
+ val tableConfigKeyGen = if
(tableConfig.getProps.containsKey(HoodieTableConfig.KEY_GENERATOR_TYPE)) {
+
KeyGeneratorType.valueOf(tableConfig.getString(HoodieTableConfig.KEY_GENERATOR_TYPE)).getClassName
+ } else {
+ tableConfig.getString(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME)
Review Comment:
refactor to reuse utilities from `KeyGeneratorType` instead.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieWriterUtils.scala:
##########
@@ -194,7 +194,11 @@ object HoodieWriterUtils {
}
val datasourceKeyGen = getOriginKeyGenerator(params)
- val tableConfigKeyGen =
tableConfig.getString(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME)
+ val tableConfigKeyGen = if
(tableConfig.getProps.containsKey(HoodieTableConfig.KEY_GENERATOR_TYPE)) {
+
KeyGeneratorType.valueOf(tableConfig.getString(HoodieTableConfig.KEY_GENERATOR_TYPE)).getClassName
+ } else {
+ tableConfig.getString(HoodieTableConfig.KEY_GENERATOR_CLASS_NAME)
Review Comment:
refactor to reuse utilities from `KeyGeneratorType` instead.
--
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]
