vinothchandar commented on code in PR #13216:
URL: https://github.com/apache/hudi/pull/13216#discussion_r2078116109
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -284,7 +284,10 @@ protected void commit(HoodieTable table, String
commitActionType, String instant
}
// update Metadata table
writeTableMetadata(table, instantTime, metadata);
- activeTimeline.saveAsComplete(false,
table.getMetaClient().createNewInstant(HoodieInstant.State.INFLIGHT,
commitActionType, instantTime), Option.of(metadata));
+ HoodieInstant completedWriteInstant = activeTimeline.saveAsComplete(false,
Review Comment:
Should the `getTableFormat().commit(..)` work be pushed inside
`activeTimeline.saveAsComplete(..)` .. So from this layer, it just feels like
we are committing to the timeline per usual.. and internally it used native or
a plugged-in table format
##########
hudi-common/src/main/java/org/apache/hudi/common/PluggableTableFormat.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common;
+
+import org.apache.hudi.avro.model.HoodieCleanMetadata;
+import org.apache.hudi.common.engine.HoodieEngineContext;
+import org.apache.hudi.common.model.HoodieCommitMetadata;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.table.timeline.TimelineFactory;
+import org.apache.hudi.common.table.view.FileSystemViewManager;
+
+import java.io.Serializable;
+import java.util.List;
+
+/**
+ * External Table Format needs to implement this interface
+ */
+public interface PluggableTableFormat extends Serializable {
Review Comment:
rename: TableFormat
##########
hudi-common/src/main/java/org/apache/hudi/common/HudiPluggableTableFormat.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common;
+
+import org.apache.hudi.common.table.timeline.TimelineFactory;
+import org.apache.hudi.common.table.timeline.TimelineLayout;
+import org.apache.hudi.common.table.timeline.versioning.TimelineLayoutVersion;
+
+public class HudiPluggableTableFormat implements PluggableTableFormat {
Review Comment:
rename: `NativeTableFormat`
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java:
##########
@@ -453,6 +454,7 @@ HoodieTableMetaClient
initializeEmptyTable(HoodieTableMetaClient.TableBuilder ta
Boolean.parseBoolean(HIVE_STYLE_PARTITIONING_ENABLE.defaultValue())))
.setUrlEncodePartitioning(props.getBoolean(URL_ENCODE_PARTITIONING.key(),
Boolean.parseBoolean(URL_ENCODE_PARTITIONING.defaultValue())))
+
.setPluggableTableFormatClass(props.getProperty(HoodieTableConfig.SUPPLEMENTARY_TABLE_FORMAT_CLASS_NAME.key(),
HudiPluggableTableFormat.class.getName()))
Review Comment:
For tests, I think we should implement a `TestTableFormat`?
and +1 on making all write paths and read paths respect this.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -198,7 +200,17 @@ public class HoodieTableConfig extends HoodieConfig {
.key("hoodie.timeline.layout.version")
.noDefaultValue()
.withDocumentation("Version of timeline used, by the table.");
-
+
+ public static final ConfigProperty<String>
SUPPLEMENTARY_TABLE_FORMAT_CLASS_NAME = ConfigProperty
Review Comment:
drop "supplementary" etc..
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -198,7 +200,17 @@ public class HoodieTableConfig extends HoodieConfig {
.key("hoodie.timeline.layout.version")
.noDefaultValue()
.withDocumentation("Version of timeline used, by the table.");
-
+
+ public static final ConfigProperty<String>
SUPPLEMENTARY_TABLE_FORMAT_CLASS_NAME = ConfigProperty
+ .key("hoodie.table.supplementary.format.class.name")
Review Comment:
rename: `hoodie.table.format` and it takes a string/enum.. defaults to
`native`
lets not add any class names into table property.. lets use enums. or
strings to make table configs language agnostic.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/SkewAdjustingTimeGenerator.java:
##########
@@ -54,13 +54,14 @@ public long generateTime(boolean skipLocking) {
}
@Override
- public void consumeTime(boolean skipLocking, Consumer<Long> func) {
+ public long consumeTime(boolean skipLocking, Consumer<Long> func) {
try {
if (!skipLocking) {
lock();
}
long currentTimeMillis = generateTime(true);
func.accept(currentTimeMillis);
+ return currentTimeMillis;
} finally {
Review Comment:
use HoodieTimer?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -198,7 +200,17 @@ public class HoodieTableConfig extends HoodieConfig {
.key("hoodie.timeline.layout.version")
.noDefaultValue()
.withDocumentation("Version of timeline used, by the table.");
-
+
+ public static final ConfigProperty<String>
SUPPLEMENTARY_TABLE_FORMAT_CLASS_NAME = ConfigProperty
+ .key("hoodie.table.supplementary.format.class.name")
+ .defaultValue(HudiPluggableTableFormat.class.getName())
+ .withDocumentation("Supplementary format name used when writing to the
table.");
+
+ public static final ConfigProperty<String>
SUPPLEMENTARY_TABLE_FORMAT_VERSION = ConfigProperty
+ .key("hoodie.table.supplementary.format.version")
Review Comment:
`hoodie.table.format.version` ? is this same as `hoodie.table.version`?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/versioning/v1/TimelineArchiverV1.java:
##########
@@ -400,7 +400,8 @@ private boolean deleteArchivedInstants(List<HoodieInstant>
archivedInstants, Hoo
completedInstants.stream()
.forEach(instant ->
activeTimeline.deleteInstantFileIfExists(instant));
}
-
+ // Call Table Format archive to allow archiving in supplementary table
format.
+ table.getMetaClient().getTableFormat().archive(archivedInstants,
table.getContext(), table.getMetaClient(), table.getViewManager());
Review Comment:
same. not a fan of these extra calls everywhere, to just call the
"supplementary" table format
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -549,7 +549,9 @@ private void completeClustering(HoodieReplaceCommitMetadata
metadata,
LOG.info("Committing Clustering {} for table {}", clusteringCommitTime,
table.getConfig().getBasePath());
LOG.debug("Clustering {} finished with result {}", clusteringCommitTime,
metadata);
- ClusteringUtils.transitionClusteringOrReplaceInflightToComplete(false,
clusteringInstant, metadata, table.getActiveTimeline());
+ HoodieInstant completedClusteringInstant =
ClusteringUtils.transitionClusteringOrReplaceInflightToComplete(false,
clusteringInstant, metadata, table.getActiveTimeline());
+ // Call Table Format Commit to allow writing supplementary table format.
Review Comment:
nit: remove comment.
if we do this, the abstractions should ideally naturally flow with the code
structure.
--
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]
