vinothchandar commented on code in PR #13216:
URL: https://github.com/apache/hudi/pull/13216#discussion_r2136565955
##########
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));
+ activeTimeline.saveAsComplete(false,
+ table.getMetaClient().createNewInstant(HoodieInstant.State.INFLIGHT,
commitActionType, instantTime), Option.of(metadata),
+ completedInstant ->
table.getMetaClient().getTableFormat().commit(metadata, completedInstant,
getEngineContext(), table.getMetaClient(), table.getViewManager())
Review Comment:
nts: again, unsure if we need a call back for this.
To think through whether this can be done better
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -720,6 +728,20 @@ public Option<TimelineLayoutVersion>
getTimelineLayoutVersion() {
: Option.empty();
}
+ public TableFormat getTableFormat(TimelineLayoutVersion layoutVersion) {
Review Comment:
needs UT
##########
hudi-common/src/main/java/org/apache/hudi/common/NativeTableFormat.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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;
+import org.apache.hudi.metadata.NativeTableMetadataFactory;
+import org.apache.hudi.metadata.TableMetadataFactory;
+
+public class NativeTableFormat implements TableFormat {
+ public static final String TABLE_FORMAT = "native";
+ private final TimelineLayoutVersion timelineLayoutVersion;
+
+ public NativeTableFormat(TimelineLayoutVersion timelineLayoutVersion) {
+ this.timelineLayoutVersion = timelineLayoutVersion;
+ }
+
+ @Override
+ public String getName() {
Review Comment:
Have we implemented any "discovery" mechanism where one can implement a new
`TableFormat` .and be discovered via value returned by `getName()`
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieInstantTimeGenerator.java:
##########
@@ -151,6 +152,10 @@ public static String formatDate(Date timestamp) {
return
getInstantFromTemporalAccessor(convertDateToTemporalAccessor(timestamp));
}
Review Comment:
why do we need this specifically for purposes of pluggable tf
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -198,7 +201,12 @@ public class HoodieTableConfig extends HoodieConfig {
.key("hoodie.timeline.layout.version")
.noDefaultValue()
.withDocumentation("Version of timeline used, by the table.");
-
+
+ public static final ConfigProperty<String> TABLE_FORMAT = ConfigProperty
Review Comment:
this does not match `hoodie.table.format.plugin` as in the description?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1142,14 +1142,18 @@ private void
clearMetadataTablePartitionsConfig(Option<MetadataPartitionType> pa
public HoodieTableMetadata getMetadataTable() {
if (metadata == null) {
- HoodieMetadataConfig metadataConfig = HoodieMetadataConfig.newBuilder()
- .fromProperties(config.getMetadataConfig().getProps())
- .build();
- metadata = HoodieTableMetadata.create(context, metaClient.getStorage(),
metadataConfig, config.getBasePath());
+ metadata = refreshMetadataTable();
}
return metadata;
}
+ public HoodieTableMetadata refreshMetadataTable() {
Review Comment:
i think this is better named `newTableMetadata()` since this does not update
the `metadata` member anyway
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/CopyOnWriteRollbackActionExecutor.java:
##########
@@ -67,11 +67,24 @@ protected List<HoodieRollbackStat>
executeRollback(HoodieRollbackPlan hoodieRoll
if (instantToRollback.isCompleted()) {
LOG.info("Unpublishing instant " + instantToRollback);
+ // rollback instant in supplementary table format first.
Review Comment:
remove `supplementary` language..
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -514,13 +523,22 @@ public synchronized void reload() {
reloadTableConfig();
}
+ /**
+ * Get the active instants as a timeline in native format.
+ *
+ * @return Active instants timeline
+ */
+ public synchronized HoodieActiveTimeline getActiveTimelineForNativeFormat() {
+ return new
NativeTableFormat(activeTimeline.getTimelineLayoutVersion()).getTimelineFactory().createActiveTimeline(this);
Review Comment:
why the hardcoding to NativeTableFormat
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1142,14 +1142,18 @@ private void
clearMetadataTablePartitionsConfig(Option<MetadataPartitionType> pa
public HoodieTableMetadata getMetadataTable() {
Review Comment:
rename: tableMetadata()
##########
hudi-common/src/main/java/org/apache/hudi/common/fs/FSUtils.java:
##########
@@ -256,7 +257,8 @@ public static List<String>
getAllPartitionPaths(HoodieEngineContext engineContex
HoodieStorage storage,
Review Comment:
Want to check if these additional `metaClient` instantiations can be avoided
by passing stuff in from calling..
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TableFormatAction.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.table.timeline;
+
+
+/**
+ * Functional Interface for executing table format actions.
+ */
+@FunctionalInterface
+public interface TableFormatAction {
Review Comment:
should atleast call this . `TableFormatCompletionAction`
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -549,7 +549,8 @@ 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());
+ ClusteringUtils.transitionClusteringOrReplaceInflightToComplete(false,
clusteringInstant, metadata, table.getActiveTimeline(),
Review Comment:
nts: can all this be pushed inside the method itself
##########
hudi-common/src/main/java/org/apache/hudi/common/NativeTableFormat.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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;
+import org.apache.hudi.metadata.NativeTableMetadataFactory;
+import org.apache.hudi.metadata.TableMetadataFactory;
+
+public class NativeTableFormat implements TableFormat {
+ public static final String TABLE_FORMAT = "native";
+ private final TimelineLayoutVersion timelineLayoutVersion;
+
+ public NativeTableFormat(TimelineLayoutVersion timelineLayoutVersion) {
+ this.timelineLayoutVersion = timelineLayoutVersion;
+ }
+
+ @Override
+ public String getName() {
Review Comment:
this should be used and not `TABLE_FORMAT` directly.. make that private?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala:
##########
@@ -324,6 +324,7 @@ class HoodieSparkSqlWriterInternal {
.setRecordMergeStrategyId(recordMergeStrategyId)
.setRecordMergeMode(RecordMergeMode.getValue(hoodieConfig.getString(HoodieWriteConfig.RECORD_MERGE_MODE)))
.setMultipleBaseFileFormatsEnabled(hoodieConfig.getBoolean(HoodieTableConfig.MULTIPLE_BASE_FILE_FORMATS_ENABLE))
+
.setTableFormat(hoodieConfig.getStringOrDefault(HoodieTableConfig.TABLE_FORMAT))
Review Comment:
can we use Spark SQL with a plugged in TF. can we add tests for basic
queries and DML
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/stats/FileStatsIndex.java:
##########
@@ -102,9 +103,11 @@ public String getIndexPartitionName() {
public HoodieTableMetadata getMetadataTable() {
// initialize the metadata table lazily
if (this.metadataTable == null) {
- this.metadataTable = HoodieTableMetadata.create(
+ HoodieHadoopStorage storage = new HoodieHadoopStorage(basePath,
FlinkClientUtil.getHadoopConf());
+ HoodieTableMetaClient metaClient =
HoodieTableMetaClient.builder().setBasePath(basePath).setConf(storage.getConf()).build();
Review Comment:
we don't have a `metaClient` available here readily?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/versioning/v1/TimelineArchiverV1.java:
##########
@@ -297,7 +297,7 @@ private List<HoodieInstant> getInstantsToArchive() throws
IOException {
// If metadata table is enabled, do not archive instants which are more
recent than the last compaction on the
// metadata table.
if (config.isMetadataTableEnabled() &&
table.getMetaClient().getTableConfig().isMetadataTableAvailable()) {
- try (HoodieTableMetadata tableMetadata =
HoodieTableMetadata.create(table.getContext(), table.getStorage(),
config.getMetadataConfig(), config.getBasePath())) {
+ try (HoodieTableMetadata tableMetadata = table.refreshMetadataTable()) {
Review Comment:
rename: refreshAndGetTableMetadata()
--
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]
