vinothchandar commented on a change in pull request #2342:
URL: https://github.com/apache/hudi/pull/2342#discussion_r546309417



##########
File path: 
hudi-client/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
##########
@@ -266,7 +256,7 @@ private void initialize(JavaSparkContext jsc, 
HoodieTableMetaClient datasetMetaC
   }
 
   private void initTableMetadata() {
-    this.metadata = new HoodieBackedTableMetadata(hadoopConf.get(), 
datasetWriteConfig.getBasePath(), datasetWriteConfig.getSpillableMapBasePath(),
+    this.metadata = (HoodieBackedTableMetadata) 
AbstractHoodieTableMetadata.create(hadoopConf.get(), 
datasetWriteConfig.getBasePath(), datasetWriteConfig.getSpillableMapBasePath(),

Review comment:
       I think we can keep the factory method in the interface?

##########
File path: 
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java
##########
@@ -66,24 +58,13 @@
  * If the metadata table does not exist, RPC calls are used to retrieve file 
listings from the file system.
  * No updates are applied to the table and it is not synced.
  */
-public class HoodieBackedTableMetadata implements HoodieTableMetadata {
+public class HoodieBackedTableMetadata extends AbstractHoodieTableMetadata {

Review comment:
       Given the only subclass of `AbstractHoodieTableMetadata` is this class 
and the abstract class already assumes the existence of the metadata table, I 
am not sure if the splitting is needed per se. 
   
   I was under the impression that we would like to implement the timeline 
merging as another subclass. If we stick with the new scanner approach (which I 
actually have grown to like actually), we no longer need the abstract class 
right? 

##########
File path: 
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
##########
@@ -0,0 +1,313 @@
+/*
+ * 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.metadata;
+
+import org.apache.hadoop.fs.Path;
+import org.apache.hudi.avro.model.HoodieCleanMetadata;
+import org.apache.hudi.avro.model.HoodieCleanerPlan;
+import org.apache.hudi.avro.model.HoodieRestoreMetadata;
+import org.apache.hudi.avro.model.HoodieRollbackMetadata;
+import org.apache.hudi.common.model.HoodieCommitMetadata;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.table.timeline.TimelineMetadataUtils;
+import org.apache.hudi.common.util.CleanerUtils;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.common.util.ValidationUtils;
+import org.apache.hudi.exception.HoodieException;
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import static 
org.apache.hudi.metadata.HoodieTableMetadata.NON_PARTITIONED_NAME;
+
+/**
+ * A utility to convert timeline information to metadata table records.
+ */
+public class HoodieTableMetadataUtil {
+
+  private static final Logger LOG = 
LogManager.getLogger(HoodieTableMetadataUtil.class);
+
+  /**
+   * Converts a timeline instant to metadata table records.
+   *
+   * @param datasetMetaClient The meta client associated with the timeline 
instant
+   * @param instant to fetch and convert to metadata table records
+   * @return a list of metadata table records
+   * @throws IOException
+   */
+  public static Option<List<HoodieRecord>> 
convertInstantToMetaRecords(HoodieTableMetaClient datasetMetaClient, 
HoodieInstant instant) throws IOException {

Review comment:
       assume most of this code is just from the writer class verbatim

##########
File path: 
hudi-client/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
##########
@@ -569,114 +471,7 @@ public void update(HoodieRollbackMetadata 
rollbackMetadata, String instantTime)
       return;
     }
 
-    Map<String, Map<String, Long>> partitionToAppendedFiles = new HashMap<>();
-    Map<String, List<String>> partitionToDeletedFiles = new HashMap<>();
-    processRollbackMetadata(rollbackMetadata, partitionToDeletedFiles, 
partitionToAppendedFiles);
-    commitRollback(jsc, partitionToDeletedFiles, partitionToAppendedFiles, 
instantTime, "Rollback");
-  }
-
-  /**
-   * Extracts information about the deleted and append files from the {@code 
HoodieRollbackMetadata}.
-   *
-   * During a rollback files may be deleted (COW, MOR) or rollback blocks be 
appended (MOR only) to files. This
-   * function will extract this change file for each partition.
-   *
-   * @param rollbackMetadata {@code HoodieRollbackMetadata}
-   * @param partitionToDeletedFiles The {@code Map} to fill with files deleted 
per partition.
-   * @param partitionToAppendedFiles The {@code Map} to fill with files 
appended per partition and their sizes.
-   */
-  private void processRollbackMetadata(HoodieRollbackMetadata rollbackMetadata,
-                                       Map<String, List<String>> 
partitionToDeletedFiles,
-                                       Map<String, Map<String, Long>> 
partitionToAppendedFiles) {
-    rollbackMetadata.getPartitionMetadata().values().forEach(pm -> {
-      final String partition = pm.getPartitionPath();
-
-      if (!pm.getSuccessDeleteFiles().isEmpty()) {
-        if (!partitionToDeletedFiles.containsKey(partition)) {
-          partitionToDeletedFiles.put(partition, new ArrayList<>());
-        }
-
-        // Extract deleted file name from the absolute paths saved in 
getSuccessDeleteFiles()
-        List<String> deletedFiles = pm.getSuccessDeleteFiles().stream().map(p 
-> new Path(p).getName())
-            .collect(Collectors.toList());
-        partitionToDeletedFiles.get(partition).addAll(deletedFiles);
-      }
-
-      if (!pm.getAppendFiles().isEmpty()) {
-        if (!partitionToAppendedFiles.containsKey(partition)) {
-          partitionToAppendedFiles.put(partition, new HashMap<>());
-        }
-
-        // Extract appended file name from the absolute paths saved in 
getAppendFiles()
-        pm.getAppendFiles().forEach((path, size) -> {
-          partitionToAppendedFiles.get(partition).merge(new 
Path(path).getName(), size, (oldSize, newSizeCopy) -> {
-            return size + oldSize;
-          });
-        });
-      }
-    });
-  }
-
-  /**
-   * Create file delete records and commit.
-   *
-   * @param partitionToDeletedFiles {@code Map} of partitions and the deleted 
files
-   * @param instantTime Timestamp at which the deletes took place
-   * @param operation Type of the operation which caused the files to be 
deleted
-   */
-  private void commitRollback(JavaSparkContext jsc, Map<String, List<String>> 
partitionToDeletedFiles,
-                              Map<String, Map<String, Long>> 
partitionToAppendedFiles, String instantTime,
-                              String operation) {
-    List<HoodieRecord> records = new LinkedList<>();
-    int[] fileChangeCount = {0, 0}; // deletes, appends
-
-    partitionToDeletedFiles.forEach((partition, deletedFiles) -> {
-      // Rollbacks deletes instants from timeline. The instant being 
rolled-back may not have been synced to the

Review comment:
       left a comment around this scenario. there is one valid case here. good 
call out in the summary

##########
File path: 
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java
##########
@@ -117,172 +93,15 @@ public HoodieBackedTableMetadata(Configuration conf, 
String datasetBasePath, Str
     } else {
       LOG.info("Metadata table is disabled.");
     }
-

Review comment:
       I assume all of this code, is just verbatim moved up to the base class ?

##########
File path: 
hudi-client/src/test/java/org/apache/hudi/metadata/TestHoodieBackedMetadata.java
##########
@@ -619,32 +618,97 @@ public void testMetadataMetrics() throws Exception {
     }
   }
 
+  //@ParameterizedTest
+  //@EnumSource(HoodieTableType.class)
+  public void testMetadataOutOfSync(HoodieTableType tableType) throws 
Exception {
+    init(HoodieTableType.COPY_ON_WRITE);
+
+    HoodieWriteClient unsyncedClient = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, true));
+
+    // Enable metadata so table is initialized
+    try (HoodieWriteClient client = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, true))) {
+      // Perform Bulk Insert
+      String newCommitTime = "001";
+      client.startCommitWithTime(newCommitTime);
+      List<HoodieRecord> records = dataGen.generateInserts(newCommitTime, 20);
+      client.bulkInsert(jsc.parallelize(records, 1), newCommitTime).collect();
+    }
+
+    // Perform commit operations with metadata disabled
+    try (HoodieWriteClient client = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, false))) {
+      // Perform Insert
+      String newCommitTime = "002";
+      client.startCommitWithTime(newCommitTime);
+      List<HoodieRecord> records = dataGen.generateInserts(newCommitTime, 20);
+      client.insert(jsc.parallelize(records, 1), newCommitTime).collect();
+
+      // Perform Upsert
+      newCommitTime = "003";
+      client.startCommitWithTime(newCommitTime);
+      records = dataGen.generateUniqueUpdates(newCommitTime, 20);
+      client.upsert(jsc.parallelize(records, 1), newCommitTime).collect();
+
+      // Compaction
+      if (metaClient.getTableType() == HoodieTableType.MERGE_ON_READ) {
+        newCommitTime = "004";
+        client.scheduleCompactionAtInstant(newCommitTime, Option.empty());
+        client.compact(newCommitTime);
+      }
+    }
+
+    assertFalse(metadata(unsyncedClient).isInSync());
+    validateMetadata(unsyncedClient);
+
+    // Perform clean operation with metadata disabled
+    try (HoodieWriteClient client = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, false))) {
+      // One more commit needed to trigger clean so upsert and compact
+      String newCommitTime = "005";
+      client.startCommitWithTime(newCommitTime);
+      List<HoodieRecord> records = dataGen.generateUpdates(newCommitTime, 20);
+      client.upsert(jsc.parallelize(records, 1), newCommitTime).collect();
+
+      if (metaClient.getTableType() == HoodieTableType.MERGE_ON_READ) {
+        newCommitTime = "006";
+        client.scheduleCompactionAtInstant(newCommitTime, Option.empty());
+        client.compact(newCommitTime);
+      }
+
+      // Clean
+      newCommitTime = "007";
+      client.clean(newCommitTime);
+    }
+
+    assertFalse(metadata(unsyncedClient).isInSync());
+    validateMetadata(unsyncedClient);
+
+    // Perform restore with metadata disabled
+    try (HoodieWriteClient client = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, false))) {
+      client.restoreToInstant("004");
+    }
+
+    assertFalse(metadata(unsyncedClient).isInSync());
+    validateMetadata(unsyncedClient);
+  }
+
   /**
    * Validate the metadata tables contents to ensure it matches what is on the 
file system.
    *
    * @throws IOException
    */
   private void validateMetadata(HoodieWriteClient client) throws IOException {
     HoodieWriteConfig config = client.getConfig();
-    HoodieBackedTableMetadataWriter metadataWriter = metadataWriter(client);
-    assertNotNull(metadataWriter, "MetadataWriter should have been 
initialized");
+
+    HoodieBackedTableMetadata metadataReader = metadata(client);

Review comment:
       just `tableMetadata`? 

##########
File path: 
hudi-client/src/test/java/org/apache/hudi/metadata/TestHoodieBackedMetadata.java
##########
@@ -619,32 +618,97 @@ public void testMetadataMetrics() throws Exception {
     }
   }
 
+  //@ParameterizedTest
+  //@EnumSource(HoodieTableType.class)
+  public void testMetadataOutOfSync(HoodieTableType tableType) throws 
Exception {
+    init(HoodieTableType.COPY_ON_WRITE);
+
+    HoodieWriteClient unsyncedClient = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, true));
+
+    // Enable metadata so table is initialized
+    try (HoodieWriteClient client = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, true))) {
+      // Perform Bulk Insert
+      String newCommitTime = "001";
+      client.startCommitWithTime(newCommitTime);
+      List<HoodieRecord> records = dataGen.generateInserts(newCommitTime, 20);
+      client.bulkInsert(jsc.parallelize(records, 1), newCommitTime).collect();
+    }
+
+    // Perform commit operations with metadata disabled
+    try (HoodieWriteClient client = new HoodieWriteClient<>(jsc, 
getWriteConfig(true, false))) {
+      // Perform Insert
+      String newCommitTime = "002";
+      client.startCommitWithTime(newCommitTime);
+      List<HoodieRecord> records = dataGen.generateInserts(newCommitTime, 20);
+      client.insert(jsc.parallelize(records, 1), newCommitTime).collect();
+
+      // Perform Upsert
+      newCommitTime = "003";
+      client.startCommitWithTime(newCommitTime);
+      records = dataGen.generateUniqueUpdates(newCommitTime, 20);
+      client.upsert(jsc.parallelize(records, 1), newCommitTime).collect();
+
+      // Compaction
+      if (metaClient.getTableType() == HoodieTableType.MERGE_ON_READ) {
+        newCommitTime = "004";
+        client.scheduleCompactionAtInstant(newCommitTime, Option.empty());
+        client.compact(newCommitTime);
+      }
+    }
+
+    assertFalse(metadata(unsyncedClient).isInSync());

Review comment:
       won't metadata be synced in `postCommit()` of the last upsert above. 
wondering how this is actually not in sync

##########
File path: 
hudi-common/src/main/java/org/apache/hudi/metadata/AbstractHoodieTableMetadata.java
##########
@@ -0,0 +1,296 @@
+/*
+ * 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.metadata;
+
+import org.apache.avro.Schema;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hudi.avro.HoodieAvroUtils;
+import org.apache.hudi.avro.model.HoodieMetadataRecord;
+import org.apache.hudi.common.config.SerializableConfiguration;
+import org.apache.hudi.common.fs.FSUtils;
+import org.apache.hudi.common.metrics.Registry;
+import org.apache.hudi.common.model.HoodiePartitionMetadata;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.HoodieRecordPayload;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.util.HoodieTimer;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
+import org.apache.hudi.exception.HoodieMetadataException;
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Interface that supports querying various pieces of metadata about a hudi 
table.
+ */
+public abstract class AbstractHoodieTableMetadata implements 
HoodieTableMetadata {
+
+  private static final Logger LOG = 
LogManager.getLogger(AbstractHoodieTableMetadata.class);
+
+  static final long MAX_MEMORY_SIZE_IN_BYTES = 1024 * 1024 * 1024;
+  static final int BUFFER_SIZE = 10 * 1024 * 1024;
+
+  protected final SerializableConfiguration hadoopConf;
+  protected final Option<HoodieMetadataMetrics> metrics;
+  protected final String datasetBasePath;
+
+  // Directory used for Spillable Map when merging records
+  final String spillableMapDirectory;
+
+  protected boolean enabled;
+  private final boolean validateLookups;
+  private final boolean assumeDatePartitioning;
+
+  private transient HoodieMetadataMergedInstantRecordScanner 
timelineRecordScanner;
+
+  protected AbstractHoodieTableMetadata(Configuration hadoopConf, String 
datasetBasePath, String spillableMapDirectory,
+                                        boolean enabled, boolean 
validateLookups, boolean enableMetrics,
+                                        boolean assumeDatePartitioning) {
+    this.hadoopConf = new SerializableConfiguration(hadoopConf);
+    this.datasetBasePath = datasetBasePath;
+    this.spillableMapDirectory = spillableMapDirectory;
+
+    this.enabled = enabled;
+    this.validateLookups = validateLookups;
+    this.assumeDatePartitioning = assumeDatePartitioning;
+
+    if (enableMetrics) {
+      this.metrics = Option.of(new 
HoodieMetadataMetrics(Registry.getRegistry("HoodieMetadata")));
+    } else {
+      this.metrics = Option.empty();
+    }
+  }
+
+  public static AbstractHoodieTableMetadata create(Configuration conf, String 
datasetBasePath, String spillableMapPath, boolean useFileListingFromMetadata,
+                                                   boolean verifyListings, 
boolean enableMetrics, boolean shouldAssumeDatePartitioning) {
+    return new HoodieBackedTableMetadata(conf, datasetBasePath, 
spillableMapPath, useFileListingFromMetadata, verifyListings,
+        enableMetrics, shouldAssumeDatePartitioning);
+  }
+
+
+  /**
+   * Return the list of files in a partition.
+   *
+   * If the Metadata Table is enabled, the listing is retrieved from the 
stored metadata. Otherwise, the list of
+   * partitions is retrieved directly from the underlying {@code FileSystem}.
+   *
+   * On any errors retrieving the listing from the metadata, defaults to using 
the file system listings.
+   *
+   * @param partitionPath The absolute path of the partition to list
+   */
+
+  public FileStatus[] getAllFilesInPartition(Path partitionPath) throws 
IOException {
+    if (enabled) {
+      try {
+        return fetchAllFilesInPartition(partitionPath);
+      } catch (Exception e) {
+        LOG.error("Failed to retrive files in partition " + partitionPath + " 
from metadata", e);
+      }
+    }
+    return FSUtils.getFs(partitionPath.toString(), 
hadoopConf.get()).listStatus(partitionPath);
+  }
+
+  /**
+   * Return the list of partitions in the dataset.
+   *
+   * If the Metadata Table is enabled, the listing is retrieved from the 
stored metadata. Otherwise, the list of
+   * partitions is retrieved directly from the underlying {@code FileSystem}.
+   *
+   * On any errors retrieving the listing from the metadata, defaults to using 
the file system listings.
+   *
+   */
+  public List<String> getAllPartitionPaths() throws IOException {
+    if (enabled) {
+      try {
+        return fetchAllPartitionPaths();
+      } catch (Exception e) {
+        LOG.error("Failed to retrieve list of partition from metadata", e);
+      }
+    }
+
+    FileSystem fs = FSUtils.getFs(datasetBasePath, hadoopConf.get());
+    return FSUtils.getAllPartitionPaths(fs, datasetBasePath, 
assumeDatePartitioning);
+  }
+
+  /**
+   * Returns a list of all partitions.
+   */
+  /**
+   * Returns a list of all partitions.
+   */
+  protected List<String> fetchAllPartitionPaths() throws IOException {
+    HoodieTimer timer = new HoodieTimer().startTimer();
+    Option<HoodieRecord<HoodieMetadataPayload>> hoodieRecord = 
getMergedRecordByKey(RECORDKEY_PARTITION_LIST);
+    metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.LOOKUP_PARTITIONS_STR, timer.endTimer()));
+
+    List<String> partitions = Collections.emptyList();
+    if (hoodieRecord.isPresent()) {
+      if (!hoodieRecord.get().getData().getDeletions().isEmpty()) {
+        throw new HoodieMetadataException("Metadata partition list record is 
inconsistent: "
+                + hoodieRecord.get().getData());
+      }
+
+      partitions = hoodieRecord.get().getData().getFilenames();
+      // Partition-less tables have a single empty partition
+      if (partitions.contains(NON_PARTITIONED_NAME)) {
+        partitions.remove(NON_PARTITIONED_NAME);
+        partitions.add("");
+      }
+    }
+
+    if (validateLookups) {
+      // Validate the Metadata Table data by listing the partitions from the 
file system
+      timer.startTimer();
+      HoodieTableMetaClient metaClient = new 
HoodieTableMetaClient(hadoopConf.get(), datasetBasePath);
+      List<String> actualPartitions  = 
FSUtils.getAllPartitionPaths(metaClient.getFs(), datasetBasePath, false);
+      metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_PARTITIONS_STR, 
timer.endTimer()));
+
+      Collections.sort(actualPartitions);
+      Collections.sort(partitions);
+      if (!actualPartitions.equals(partitions)) {
+        LOG.error("Validation of metadata partition list failed. Lists do not 
match.");
+        LOG.error("Partitions from metadata: " + 
Arrays.toString(partitions.toArray()));
+        LOG.error("Partitions from file system: " + 
Arrays.toString(actualPartitions.toArray()));
+
+        metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_ERRORS_STR, 0));
+      }
+
+      // Return the direct listing as it should be correct
+      partitions = actualPartitions;
+    }
+
+    LOG.info("Listed partitions from metadata: #partitions=" + 
partitions.size());
+    return partitions;
+  }
+
+  /**
+   * Return all the files from the partition.
+   *
+   * @param partitionPath The absolute path of the partition
+   */
+  private FileStatus[] fetchAllFilesInPartition(Path partitionPath) throws 
IOException {
+    String partitionName = FSUtils.getRelativePartitionPath(new 
Path(datasetBasePath), partitionPath);
+    if (partitionName.isEmpty()) {
+      partitionName = NON_PARTITIONED_NAME;
+    }
+
+    HoodieTimer timer = new HoodieTimer().startTimer();
+    Option<HoodieRecord<HoodieMetadataPayload>> hoodieRecord = 
getMergedRecordByKey(partitionName);
+    metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.LOOKUP_FILES_STR, timer.endTimer()));
+
+    FileStatus[] statuses = {};
+    if (hoodieRecord.isPresent()) {
+      statuses = hoodieRecord.get().getData().getFileStatuses(partitionPath);
+    }
+
+    if (validateLookups) {
+      // Validate the Metadata Table data by listing the partitions from the 
file system
+      timer.startTimer();
+
+      // Ignore partition metadata file
+      HoodieTableMetaClient metaClient = new 
HoodieTableMetaClient(hadoopConf.get(), datasetBasePath);
+      FileStatus[] directStatuses = 
metaClient.getFs().listStatus(partitionPath,
+          p -> 
!p.getName().equals(HoodiePartitionMetadata.HOODIE_PARTITION_METAFILE));
+      metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_FILES_STR, timer.endTimer()));
+
+      List<String> directFilenames = Arrays.stream(directStatuses)
+              .map(s -> s.getPath().getName()).sorted()
+              .collect(Collectors.toList());
+
+      List<String> metadataFilenames = Arrays.stream(statuses)
+              .map(s -> s.getPath().getName()).sorted()
+              .collect(Collectors.toList());
+
+      if (!metadataFilenames.equals(directFilenames)) {
+        LOG.error("Validation of metadata file listing for partition " + 
partitionName + " failed.");
+        LOG.error("File list from metadata: " + 
Arrays.toString(metadataFilenames.toArray()));
+        LOG.error("File list from direct listing: " + 
Arrays.toString(directFilenames.toArray()));
+
+        metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_ERRORS_STR, 0));
+      }
+
+      // Return the direct listing as it should be correct
+      statuses = directStatuses;
+    }
+
+    LOG.info("Listed file in partition from metadata: partition=" + 
partitionName + ", #files=" + statuses.length);
+    return statuses;
+  }
+
+  /**
+   * Retrieve the merged {@code HoodieRecord} mapped to the given key.
+   *
+   * @param key The key of the record
+   */
+  private Option<HoodieRecord<HoodieMetadataPayload>> 
getMergedRecordByKey(String key) throws IOException {
+    openTimelineScanner();
+
+    Option<HoodieRecord<HoodieMetadataPayload>> metadataHoodieRecord = 
getRecordByKeyFromMetadata(key);
+
+    // Retrieve record from unsynced timeline instants
+    Option<HoodieRecord<HoodieMetadataPayload>> timelineHoodieRecord = 
timelineRecordScanner.getRecordByKey(key);
+    if (timelineHoodieRecord.isPresent()) {
+      if (metadataHoodieRecord.isPresent()) {
+        HoodieRecordPayload mergedPayload = 
timelineHoodieRecord.get().getData().preCombine(metadataHoodieRecord.get().getData());
+        metadataHoodieRecord = Option.of(new 
HoodieRecord(metadataHoodieRecord.get().getKey(), mergedPayload));
+      } else {
+        metadataHoodieRecord = timelineHoodieRecord;

Review comment:
       just return out of here, instead of reassigning to another important 
variable. may be easier to read.

##########
File path: 
hudi-common/src/main/java/org/apache/hudi/metadata/AbstractHoodieTableMetadata.java
##########
@@ -0,0 +1,296 @@
+/*
+ * 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.metadata;
+
+import org.apache.avro.Schema;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hudi.avro.HoodieAvroUtils;
+import org.apache.hudi.avro.model.HoodieMetadataRecord;
+import org.apache.hudi.common.config.SerializableConfiguration;
+import org.apache.hudi.common.fs.FSUtils;
+import org.apache.hudi.common.metrics.Registry;
+import org.apache.hudi.common.model.HoodiePartitionMetadata;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.HoodieRecordPayload;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.util.HoodieTimer;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.Path;
+import org.apache.hudi.exception.HoodieMetadataException;
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Interface that supports querying various pieces of metadata about a hudi 
table.
+ */
+public abstract class AbstractHoodieTableMetadata implements 
HoodieTableMetadata {
+
+  private static final Logger LOG = 
LogManager.getLogger(AbstractHoodieTableMetadata.class);
+
+  static final long MAX_MEMORY_SIZE_IN_BYTES = 1024 * 1024 * 1024;
+  static final int BUFFER_SIZE = 10 * 1024 * 1024;
+
+  protected final SerializableConfiguration hadoopConf;
+  protected final Option<HoodieMetadataMetrics> metrics;
+  protected final String datasetBasePath;
+
+  // Directory used for Spillable Map when merging records
+  final String spillableMapDirectory;
+
+  protected boolean enabled;
+  private final boolean validateLookups;
+  private final boolean assumeDatePartitioning;
+
+  private transient HoodieMetadataMergedInstantRecordScanner 
timelineRecordScanner;
+
+  protected AbstractHoodieTableMetadata(Configuration hadoopConf, String 
datasetBasePath, String spillableMapDirectory,
+                                        boolean enabled, boolean 
validateLookups, boolean enableMetrics,
+                                        boolean assumeDatePartitioning) {
+    this.hadoopConf = new SerializableConfiguration(hadoopConf);
+    this.datasetBasePath = datasetBasePath;
+    this.spillableMapDirectory = spillableMapDirectory;
+
+    this.enabled = enabled;
+    this.validateLookups = validateLookups;
+    this.assumeDatePartitioning = assumeDatePartitioning;
+
+    if (enableMetrics) {
+      this.metrics = Option.of(new 
HoodieMetadataMetrics(Registry.getRegistry("HoodieMetadata")));
+    } else {
+      this.metrics = Option.empty();
+    }
+  }
+
+  public static AbstractHoodieTableMetadata create(Configuration conf, String 
datasetBasePath, String spillableMapPath, boolean useFileListingFromMetadata,
+                                                   boolean verifyListings, 
boolean enableMetrics, boolean shouldAssumeDatePartitioning) {
+    return new HoodieBackedTableMetadata(conf, datasetBasePath, 
spillableMapPath, useFileListingFromMetadata, verifyListings,
+        enableMetrics, shouldAssumeDatePartitioning);
+  }
+
+
+  /**
+   * Return the list of files in a partition.
+   *
+   * If the Metadata Table is enabled, the listing is retrieved from the 
stored metadata. Otherwise, the list of
+   * partitions is retrieved directly from the underlying {@code FileSystem}.
+   *
+   * On any errors retrieving the listing from the metadata, defaults to using 
the file system listings.
+   *
+   * @param partitionPath The absolute path of the partition to list
+   */
+
+  public FileStatus[] getAllFilesInPartition(Path partitionPath) throws 
IOException {
+    if (enabled) {
+      try {
+        return fetchAllFilesInPartition(partitionPath);
+      } catch (Exception e) {
+        LOG.error("Failed to retrive files in partition " + partitionPath + " 
from metadata", e);
+      }
+    }
+    return FSUtils.getFs(partitionPath.toString(), 
hadoopConf.get()).listStatus(partitionPath);
+  }
+
+  /**
+   * Return the list of partitions in the dataset.
+   *
+   * If the Metadata Table is enabled, the listing is retrieved from the 
stored metadata. Otherwise, the list of
+   * partitions is retrieved directly from the underlying {@code FileSystem}.
+   *
+   * On any errors retrieving the listing from the metadata, defaults to using 
the file system listings.
+   *
+   */
+  public List<String> getAllPartitionPaths() throws IOException {
+    if (enabled) {
+      try {
+        return fetchAllPartitionPaths();
+      } catch (Exception e) {
+        LOG.error("Failed to retrieve list of partition from metadata", e);
+      }
+    }
+
+    FileSystem fs = FSUtils.getFs(datasetBasePath, hadoopConf.get());
+    return FSUtils.getAllPartitionPaths(fs, datasetBasePath, 
assumeDatePartitioning);
+  }
+
+  /**
+   * Returns a list of all partitions.
+   */
+  /**
+   * Returns a list of all partitions.
+   */
+  protected List<String> fetchAllPartitionPaths() throws IOException {
+    HoodieTimer timer = new HoodieTimer().startTimer();
+    Option<HoodieRecord<HoodieMetadataPayload>> hoodieRecord = 
getMergedRecordByKey(RECORDKEY_PARTITION_LIST);
+    metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.LOOKUP_PARTITIONS_STR, timer.endTimer()));
+
+    List<String> partitions = Collections.emptyList();
+    if (hoodieRecord.isPresent()) {
+      if (!hoodieRecord.get().getData().getDeletions().isEmpty()) {
+        throw new HoodieMetadataException("Metadata partition list record is 
inconsistent: "
+                + hoodieRecord.get().getData());
+      }
+
+      partitions = hoodieRecord.get().getData().getFilenames();
+      // Partition-less tables have a single empty partition
+      if (partitions.contains(NON_PARTITIONED_NAME)) {
+        partitions.remove(NON_PARTITIONED_NAME);
+        partitions.add("");
+      }
+    }
+
+    if (validateLookups) {
+      // Validate the Metadata Table data by listing the partitions from the 
file system
+      timer.startTimer();
+      HoodieTableMetaClient metaClient = new 
HoodieTableMetaClient(hadoopConf.get(), datasetBasePath);
+      List<String> actualPartitions  = 
FSUtils.getAllPartitionPaths(metaClient.getFs(), datasetBasePath, false);
+      metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_PARTITIONS_STR, 
timer.endTimer()));
+
+      Collections.sort(actualPartitions);
+      Collections.sort(partitions);
+      if (!actualPartitions.equals(partitions)) {
+        LOG.error("Validation of metadata partition list failed. Lists do not 
match.");
+        LOG.error("Partitions from metadata: " + 
Arrays.toString(partitions.toArray()));
+        LOG.error("Partitions from file system: " + 
Arrays.toString(actualPartitions.toArray()));
+
+        metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_ERRORS_STR, 0));
+      }
+
+      // Return the direct listing as it should be correct
+      partitions = actualPartitions;
+    }
+
+    LOG.info("Listed partitions from metadata: #partitions=" + 
partitions.size());
+    return partitions;
+  }
+
+  /**
+   * Return all the files from the partition.
+   *
+   * @param partitionPath The absolute path of the partition
+   */
+  private FileStatus[] fetchAllFilesInPartition(Path partitionPath) throws 
IOException {
+    String partitionName = FSUtils.getRelativePartitionPath(new 
Path(datasetBasePath), partitionPath);
+    if (partitionName.isEmpty()) {
+      partitionName = NON_PARTITIONED_NAME;
+    }
+
+    HoodieTimer timer = new HoodieTimer().startTimer();
+    Option<HoodieRecord<HoodieMetadataPayload>> hoodieRecord = 
getMergedRecordByKey(partitionName);
+    metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.LOOKUP_FILES_STR, timer.endTimer()));
+
+    FileStatus[] statuses = {};
+    if (hoodieRecord.isPresent()) {
+      statuses = hoodieRecord.get().getData().getFileStatuses(partitionPath);
+    }
+
+    if (validateLookups) {
+      // Validate the Metadata Table data by listing the partitions from the 
file system
+      timer.startTimer();
+
+      // Ignore partition metadata file
+      HoodieTableMetaClient metaClient = new 
HoodieTableMetaClient(hadoopConf.get(), datasetBasePath);
+      FileStatus[] directStatuses = 
metaClient.getFs().listStatus(partitionPath,
+          p -> 
!p.getName().equals(HoodiePartitionMetadata.HOODIE_PARTITION_METAFILE));
+      metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_FILES_STR, timer.endTimer()));
+
+      List<String> directFilenames = Arrays.stream(directStatuses)
+              .map(s -> s.getPath().getName()).sorted()
+              .collect(Collectors.toList());
+
+      List<String> metadataFilenames = Arrays.stream(statuses)
+              .map(s -> s.getPath().getName()).sorted()
+              .collect(Collectors.toList());
+
+      if (!metadataFilenames.equals(directFilenames)) {
+        LOG.error("Validation of metadata file listing for partition " + 
partitionName + " failed.");
+        LOG.error("File list from metadata: " + 
Arrays.toString(metadataFilenames.toArray()));
+        LOG.error("File list from direct listing: " + 
Arrays.toString(directFilenames.toArray()));
+
+        metrics.ifPresent(m -> 
m.updateMetrics(HoodieMetadataMetrics.VALIDATE_ERRORS_STR, 0));
+      }
+
+      // Return the direct listing as it should be correct
+      statuses = directStatuses;
+    }
+
+    LOG.info("Listed file in partition from metadata: partition=" + 
partitionName + ", #files=" + statuses.length);
+    return statuses;
+  }
+
+  /**
+   * Retrieve the merged {@code HoodieRecord} mapped to the given key.
+   *
+   * @param key The key of the record
+   */
+  private Option<HoodieRecord<HoodieMetadataPayload>> 
getMergedRecordByKey(String key) throws IOException {
+    openTimelineScanner();
+
+    Option<HoodieRecord<HoodieMetadataPayload>> metadataHoodieRecord = 
getRecordByKeyFromMetadata(key);
+
+    // Retrieve record from unsynced timeline instants
+    Option<HoodieRecord<HoodieMetadataPayload>> timelineHoodieRecord = 
timelineRecordScanner.getRecordByKey(key);
+    if (timelineHoodieRecord.isPresent()) {
+      if (metadataHoodieRecord.isPresent()) {
+        HoodieRecordPayload mergedPayload = 
timelineHoodieRecord.get().getData().preCombine(metadataHoodieRecord.get().getData());
+        metadataHoodieRecord = Option.of(new 
HoodieRecord(metadataHoodieRecord.get().getKey(), mergedPayload));
+      } else {
+        metadataHoodieRecord = timelineHoodieRecord;
+      }
+    }
+
+    return metadataHoodieRecord;
+  }
+
+  protected abstract Option<HoodieRecord<HoodieMetadataPayload>> 
getRecordByKeyFromMetadata(String key) throws IOException;
+
+  private void openTimelineScanner() throws IOException {
+    if (timelineRecordScanner != null) {
+      // Already opened
+      return;
+    }
+
+    HoodieTableMetaClient datasetMetaClient = new 
HoodieTableMetaClient(hadoopConf.get(), datasetBasePath);
+    List<HoodieInstant> unsyncedInstants = 
findInstantsToSync(datasetMetaClient);
+    Schema schema = 
HoodieAvroUtils.addMetadataFields(HoodieMetadataRecord.getClassSchema());
+    timelineRecordScanner =
+            new HoodieMetadataMergedInstantRecordScanner(datasetMetaClient, 
unsyncedInstants, schema, MAX_MEMORY_SIZE_IN_BYTES, spillableMapDirectory, 
null);

Review comment:
       should we pass in the filter? guess it does not matter, since we ll read 
the instants in full anyway




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to