nsivabalan commented on code in PR #13517:
URL: https://github.com/apache/hudi/pull/13517#discussion_r2229394881
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java:
##########
@@ -336,4 +350,43 @@ protected static Option<IndexedRecord>
toAvroRecord(HoodieRecord record, Schema
return Option.empty();
}
}
+
+ protected boolean
shouldTrackEventTimeWaterMarkerBasedOnMergeMode(HoodieTableMetaClient
metaClient) {
+ // Only for event time ordering mode.
+ return metaClient.getTableConfig().getRecordMergeMode() ==
RecordMergeMode.EVENT_TIME_ORDERING;
Review Comment:
lets not create methods just for 1 line
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -223,6 +224,12 @@ public class HoodieTableConfig extends HoodieConfig {
.withDocumentation("Payload class to use for performing merges,
compactions, i.e merge delta logs with current base file and then "
+ " produce a new base file.");
+ public static final ConfigProperty<String> LEGACY_PAYLOAD_CLASS_NAME =
ConfigProperty
Review Comment:
why do we have this as part of this PR?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieCreateHandle.java:
##########
@@ -133,6 +133,10 @@ public boolean canWrite(HoodieRecord record) {
@Override
protected void doWrite(HoodieRecord record, Schema schema, TypedProperties
props) {
Option<Map<String, String>> recordMetadata = record.getMetadata();
+ // Track event time metadata.
+ if (trackEventTimeWatermark && eventTimeFieldNameOpt.isPresent()) {
+ recordMetadata = appendEventTimeMetadata(record, recordMetadata);
Review Comment:
gotcha.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ConfigUtils.java:
##########
@@ -96,6 +98,31 @@ public static String getPayloadClass(Properties props) {
return HoodieRecordPayload.getPayloadClassName(props);
}
+ /**
+ * Check if event time metadata should be tracked.
+ */
+ public static boolean shouldTrackEventTimeWaterMarkByConfig(TypedProperties
props) {
Review Comment:
we can remove `ByConfig` from the suffix.
`shouldTrackEventTimeWaterMark` would suffice. entire class is about
fetching some config value. So, why bother repeating it in the method name
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java:
##########
@@ -123,6 +132,11 @@ protected HoodieWriteHandle(HoodieWriteConfig config,
String instantTime, String
} else {
this.isSecondaryIndexStatsStreamingWritesEnabled = false;
}
+
+ // For tracking event time watermark.
+ this.trackEventTimeWatermark =
shouldTrackEventTimeWaterMarker(getHoodieTableMetaClient(), config);
+ this.keepConsistentLogicalTimestamp =
ConfigUtils.shouldKeepConsistentLogicalTimestamp(config.getProps());
Review Comment:
if trackEventTimeWatermark is false, why do we need to call subsequent line
138 and 139. can we leave it to default values.
--
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]
