vinothchandar commented on code in PR #12288:
URL: https://github.com/apache/hudi/pull/12288#discussion_r1855225787
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -222,9 +222,14 @@ public class HoodieTableConfig extends HoodieConfig {
public static final ConfigProperty<String> ARCHIVELOG_FOLDER = ConfigProperty
Review Comment:
lets add a new config called `hoodie.timeline.history.path` and keep the old
one also around (but deprecated) for compact. UT this
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -579,16 +603,27 @@ private static void
createTableLayoutOnStorage(StorageConfiguration<?> storageCo
if (!storage.exists(metaPathDir)) {
storage.createDirectory(metaPathDir);
}
+
// create schema folder
StoragePath schemaPathDir = new StoragePath(metaPathDir,
SCHEMA_FOLDER_NAME);
if (!storage.exists(schemaPathDir)) {
storage.createDirectory(schemaPathDir);
}
+ // if anything other than default archive log folder is specified, create
that too
Review Comment:
UT>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/EightToSevenDowngradeHandler.java:
##########
@@ -71,6 +77,28 @@ public class EightToSevenDowngradeHandler implements
DowngradeHandler {
private static final Logger LOG =
LoggerFactory.getLogger(EightToSevenDowngradeHandler.class);
private static final Set<String> SUPPORTED_METADATA_PARTITION_PATHS =
getSupportedMetadataPartitionPaths();
+ /**
+ * Extract Epoch time from completion time string
+ * @param instant : HoodieInstant
+ * @return
+ */
+ public static long convertCompletionTimeToEpoch(HoodieInstant instant) {
Review Comment:
lets not land without unit tests.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -179,6 +183,8 @@ protected HoodieTableMetaClient(HoodieStorage storage,
String basePath, boolean
}
this.timelineLayoutVersion = layoutVersion.orElseGet(() ->
tableConfig.getTimelineLayoutVersion().get());
this.timelineLayout = TimelineLayout.fromVersion(timelineLayoutVersion);
+ this.activeTimelinePath =
timelineLayout.getTimelinePathProvider().getActiveTimelinePath(tableConfig,
this.basePath);
Review Comment:
we should just have a `timelinePath` and `timelineHistoryPath`..
- active timeline path contain the`history` or `archived` folders under
anyway. So call it timeline path.
- no mention of archived in code base.. we should consistently refer
everywhere as "history" (methods, docs, comments, configs)
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v1/TimelinePathProviderV1.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.versioning.v1;
Review Comment:
all these shuld have UTs. please dont land without them
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -477,6 +490,17 @@ public synchronized HoodieActiveTimeline
reloadActiveTimeline() {
public synchronized void reloadTableConfig() {
this.tableConfig = new HoodieTableConfig(this.storage, metaPath,
this.tableConfig.getRecordMergeMode(),
this.tableConfig.getKeyGeneratorClassName(),
this.tableConfig.getRecordMergeStrategyId());
+ reloadTimelineLayout();
+ }
+
+ /**
+ * Reload the timeline layout info.
+ */
+ private void reloadTimelineLayout() {
Review Comment:
rename. its doing more than just deal with timeline layout
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ActiveTimelineV2.java:
##########
@@ -258,8 +260,17 @@ protected String getInstantFileName(HoodieInstant instant)
{
@Override
public Option<byte[]> getInstantDetails(HoodieInstant instant) {
- StoragePath detailPath =
getInstantFileNamePath(getInstantFileName(instant));
- return readDataFromPath(detailPath);
+ try {
+ StoragePath detailPath =
getInstantFileNamePath(getInstantFileName(instant));
+ return readDataFromPath(detailPath);
+ } catch (NullPointerException npe) {
+ LOG.error("NPE. instant=" + instant);
Review Comment:
1 log message.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -222,9 +222,14 @@ public class HoodieTableConfig extends HoodieConfig {
public static final ConfigProperty<String> ARCHIVELOG_FOLDER = ConfigProperty
.key("hoodie.archivelog.folder")
- .defaultValue("archived")
+ .defaultValue("history")
.withDocumentation("path under the meta folder, to store archived
timeline instants at.");
+ public static final ConfigProperty<String> TIMELINE_FOLDER = ConfigProperty
Review Comment:
consistency of naming `hoodie.timeline.path` and also name
variables/docs/comments etc
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ActiveTimelineV2.java:
##########
@@ -258,8 +260,17 @@ protected String getInstantFileName(HoodieInstant instant)
{
@Override
public Option<byte[]> getInstantDetails(HoodieInstant instant) {
- StoragePath detailPath =
getInstantFileNamePath(getInstantFileName(instant));
- return readDataFromPath(detailPath);
+ try {
+ StoragePath detailPath =
getInstantFileNamePath(getInstantFileName(instant));
+ return readDataFromPath(detailPath);
+ } catch (NullPointerException npe) {
+ LOG.error("NPE. instant=" + instant);
Review Comment:
UT this please
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/storage/hadoop/HoodieHadoopStorage.java:
##########
@@ -212,6 +212,11 @@ public List<StoragePathInfo> listDirectEntries(StoragePath
path,
.collect(Collectors.toList());
}
+ @Override
+ public void setModificationTime(StoragePath path, long
modificationTimeInMillisEpoch) throws IOException {
Review Comment:
UT
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/EightToSevenDowngradeHandler.java:
##########
@@ -71,6 +77,28 @@ public class EightToSevenDowngradeHandler implements
DowngradeHandler {
private static final Logger LOG =
LoggerFactory.getLogger(EightToSevenDowngradeHandler.class);
private static final Set<String> SUPPORTED_METADATA_PARTITION_PATHS =
getSupportedMetadataPartitionPaths();
+ /**
+ * Extract Epoch time from completion time string
+ * @param instant : HoodieInstant
+ * @return
+ */
+ public static long convertCompletionTimeToEpoch(HoodieInstant instant) {
+ try {
+ String completionTime = instant.getCompletionTime();
+ // In Java 8, no direct API to convert to epoch in millis.
+ // Strip off millis
+ String completionTimeInSecs = completionTime.substring(0,
completionTime.length() - 3);
Review Comment:
well, convert to long and divide by 1000? or sth. this feels hacky
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -579,16 +603,27 @@ private static void
createTableLayoutOnStorage(StorageConfiguration<?> storageCo
if (!storage.exists(metaPathDir)) {
storage.createDirectory(metaPathDir);
}
+
// create schema folder
StoragePath schemaPathDir = new StoragePath(metaPathDir,
SCHEMA_FOLDER_NAME);
if (!storage.exists(schemaPathDir)) {
storage.createDirectory(schemaPathDir);
}
+ // if anything other than default archive log folder is specified, create
that too
+ String timelinePropVal = new
HoodieConfig(props).getStringOrDefault(TIMELINE_FOLDER);
+ StoragePath timelineDir = metaPathDir;
+ if (!StringUtils.isNullOrEmpty(timelinePropVal)) {
+ timelineDir = new StoragePath(metaPathDir, timelinePropVal);
+ if (!storage.exists(timelineDir)) {
Review Comment:
please avoid any of these `storage.exists()` calls in fast path.. if its
about lazily handling sth, then let it fail, and do creation on failure..
Happy path should not have these.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/TimelinePathProviderV2.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.TimelinePathProvider;
+import org.apache.hudi.storage.StoragePath;
+
+public class TimelinePathProviderV2 implements TimelinePathProvider {
Review Comment:
UT
--
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]
