danny0405 commented on code in PR #9617:
URL: https://github.com/apache/hudi/pull/9617#discussion_r1324202392
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/utils/MetadataConversionUtils.java:
##########
@@ -53,7 +53,7 @@
public class MetadataConversionUtils {
public static HoodieArchivedMetaEntry createMetaWrapper(HoodieInstant
hoodieInstant, HoodieTableMetaClient metaClient) throws IOException {
- Option<byte[]> instantDetails =
metaClient.getActiveTimeline().getInstantDetails(hoodieInstant);
+ Option<byte[]> instantDetails =
metaClient.getActiveTimeline().reload().getInstantDetails(hoodieInstant);
Review Comment:
Why we need the reload ?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/compact/CompactHelpers.java:
##########
@@ -82,7 +82,7 @@ public HoodieCommitMetadata createCompactionMetadata(
public void completeInflightCompaction(HoodieTable table, String
compactionCommitTime, HoodieCommitMetadata commitMetadata) {
HoodieActiveTimeline activeTimeline = table.getActiveTimeline();
try {
- activeTimeline.transitionCompactionInflightToComplete(
+ activeTimeline.transitionCompactionInflightToComplete(false,
table.getConfig().getTimeGeneratorConfig(),
Review Comment:
Are we sure there is no need to lock here?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java:
##########
@@ -261,7 +261,7 @@ protected void finishRollback(HoodieInstant
inflightInstant, HoodieRollbackMetad
// If publish the rollback to the timeline, we finally transition the
inflight rollback
// to complete in the data table timeline
if (!skipTimelinePublish) {
-
table.getActiveTimeline().transitionRollbackInflightToComplete(inflightInstant,
+ table.getActiveTimeline().transitionRollbackInflightToComplete(false,
table.getConfig().getTimeGeneratorConfig(), inflightInstant,
Review Comment:
false or `!enableLocking` ?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeGeneratorConfig.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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;
+
+import org.apache.hudi.common.config.ConfigProperty;
+import org.apache.hudi.common.config.HoodieConfig;
+import org.apache.hudi.common.config.LockConfiguration;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.common.util.ValidationUtils;
+
+import java.util.Properties;
+
+import static org.apache.hudi.common.config.LockConfiguration.LOCK_PREFIX;
+
+public class HoodieTimeGeneratorConfig extends HoodieConfig {
Review Comment:
We should refactor this class to be together with other config classes, can
you fire a JIRA issue to trace this?
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/TestHoodieInstantTimeGenerator.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;
+
+import org.apache.hadoop.conf.Configuration;
+
+public class TestHoodieInstantTimeGenerator {
+
+ private static HoodieTimeGeneratorConfig timeGeneratorConfig =
HoodieTimeGeneratorConfig.newBuilder()
+ .withPath("/tmp").build();
+ private static Configuration hadoopConf = new Configuration();
+
Review Comment:
Remove it or make it static final.
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/CompactionCommand.java:
##########
@@ -195,9 +196,10 @@ public String scheduleCompact(
HoodieTableMetaClient client = checkAndGetMetaClient();
boolean initialized = HoodieCLI.initConf();
HoodieCLI.initFS(initialized);
-
+ HoodieTimeGeneratorConfig timeGeneratorConfig =
HoodieTimeGeneratorConfig.newBuilder()
+ .withPath(HoodieCLI.basePath).build();
// First get a compaction instant time and pass it to spark launcher for
scheduling compaction
- String compactionInstantTime = HoodieActiveTimeline.createNewInstantTime();
+ String compactionInstantTime =
HoodieActiveTimeline.createNewInstantTime(timeGeneratorConfig, HoodieCLI.conf);
Review Comment:
Kind of think the invoke should be:
`TimeGenerators.getInstance(HoodieCLI.basePath,
HoodieCLI.conf).createNewInstantTime`
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java:
##########
@@ -304,9 +326,23 @@ protected void deleteInstantFile(HoodieInstant instant) {
}
}
+ /**
+ * Many callers might not pass completionTime, here we have to search
+ * timeline to get completionTime
+ * TODO Depreciate this method and fix relate tests
+ */
+ private String getInstantFileName(HoodieInstant instant) {
Review Comment:
You mean the `instant` passed in is constructed manually instead of filtered
from the timeline?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimeGenerator.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;
+
+public interface TimeGenerator {
+
+ void lock();
+
+ /**
+ * Generate a globally monotonically increasing timestamp. All implements
must ensure
+ * 1. any caller can compute a timestamp T that is guaranteed to be greater
than
+ * any timestamp T' if T' finished being generated before T started being
generated.
+ * 2. This guarantee holds across all different callers.
+ *
+ * @param shouldLock Whether to hold a new lock inside this method. Some
callers(like
+ * completion time generation) already holds lock, whereas here doesn't need
to lock again.
+ *
+ * @return Current true time as millis.
+ */
+ long currentTimeMillis(boolean shouldLock);
+
Review Comment:
We already have `lock` and `unlock` so do we still need the `shouldLock`
flag?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimeGeneratorType.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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;
+
+import org.apache.hudi.common.config.EnumDescription;
+import org.apache.hudi.common.config.EnumFieldDescription;
+
+/**
+ * Types of {@link TimeGenerator}.
+ */
+@EnumDescription("Time generator type, indicating the time generator class to
use, that implements "
+ + "`org.apache.hudi.common.table.timeline.TimeGenerator`.")
+public enum TimeGeneratorType {
+
+ @EnumFieldDescription("A wait based time generator, holding a mutex lock
with skew times to "
+ + "produce globally monotonically increasing timestamp")
+ WAIT_BASED
Review Comment:
`LOCK_PROVIDER` and `TRUE_TIME`
--
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]
