Small file size handling for inserts into log files. #413

n3nash · 2018-06-12T03:28:31Z

Added rolling stats to the metadata of a commit file
Determine how many inserts vs upserts from rolling stats and hence determine if more inserts can be added to a log file.
In case of absence of rolling stats, the total size of the log file is compared with the parquet max file size and if there is scope to add inserts the add it.

n3nash · 2018-06-12T07:14:16Z

@bvaradar @vinothchandar please take a pass.

bvaradar

Partially reviewed. Will review further tomorrow

bvaradar · 2018-06-25T07:37:50Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRollingStat.java

+    // called by jackson json lib
+  }
+
+  public HoodieRollingStat(String fileId, long inserts, long upserts, long totalInputWriteBytesOnDisk) {


Can you use Avro for tracking rolling stats ?

I'm not very inclined towards having an AVRO for rolling stats. The rolling stats is written to extraMetadata which is a very flexible map datastructure. Also, eventually, we are needing to have an associated java class for each avro generated class. WDYT ?

bvaradar · 2018-06-25T07:50:39Z

hoodie-client/src/test/java/com/uber/hoodie/TestHoodieClientOnCopyOnWriteStorage.java

+      }
+    }
+    Assert.assertEquals(inserts, 200);
+  }


Can you also include upsert ?

bvaradar · 2018-06-25T08:19:07Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRollingStatMetadata.java

+    return mapper;
+  }
+
+  public HoodieRollingStatMetadata merge(HoodieRollingStatMetadata rollingStatMetadata) {


Can you add a simple unit-test for merge ?

This is done via the tests of inserts + updates. Added another test for updates (which automatically tests merge). Let me know if you still feel the need for it.

bvaradar · 2018-06-25T08:21:13Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRollingStat.java

+
+  private String fileId;
+  private long inserts;
+  private long upserts;


Don't we need to track deletes to accurately track number of entries in the file ?

Good point, at the moment, pure deletes is not used but we should add it anyways.

vinothchandar · 2018-06-26T01:13:13Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRollingStat.java

+    this.upserts = upserts;
+  }
+
+  public long updateInserts(long inserts) {


rename: addInserts()/addUpserts()

vinothchandar · 2018-06-26T01:35:02Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRollingStatMetadata.java

+    }
+  }
+
+  public static HoodieRollingStatMetadata fromBytes(byte[] bytes) throws IOException {


can these methods be reused across HoodieRollingStatMetadata/HoodieCommitMetadata?

Did some refactoring.

vinothchandar · 2018-06-26T01:37:37Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRollingStatMetadata.java

+
+  public String toJsonString() throws IOException {
+    if (partitionToRollingStats.containsKey(null)) {
+      log.info("partition path is null for " + partitionToRollingStats.get(null));


do you intend to leave this logging in there forever?

Yes, this can be helpful for debugging.

vinothchandar · 2018-06-26T01:45:43Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieCopyOnWriteTable.java

    UpsertPartitioner(WorkloadProfile profile) {
      updateLocationToBucket = new HashMap<>();
      partitionPathToInsertBuckets = new HashMap<>();
      bucketInfoMap = new HashMap<>();
      globalStat = profile.getGlobalStat();
-
+      rollingStatMetadata = getRollingStats();


Now the hoodieTable initialization reading in some data as well. How expensive is this going to make it? Can you look at places where we are doing this and assess if this is an issue

Alternatively we can selectively read it out during commit & partitioning phases alone?

This is not done during hoodie table initialization. This is only done when a getUpsertPartitioner() is called on the table so it should be fine ?

okay . then its fine

vinothchandar · 2018-06-26T01:47:17Z

hoodie-client/src/main/java/com/uber/hoodie/io/HoodieAppendHandle.java

@@ -89,6 +89,8 @@
  private int maxBlockSize = config.getLogFileDataBlockMaxSize();
  // Header metadata for a log block
  private Map<HoodieLogBlock.HeaderMetadataType, String> header = Maps.newHashMap();
+  // default is false
+  private boolean isUpsert;


what does this flag signify? given its solely set based on which constructor is invoked?

Removed this, it was being used to identify upserts vs inserts to correctly set writeStatus. It is done differently now (by checking currentLocation). Please take a pass.

vinothchandar · 2018-06-26T17:06:03Z

hoodie-client/src/main/java/com/uber/hoodie/HoodieWriteClient.java

+        HoodieCommitMetadata commitMetadata = HoodieCommitMetadata
+            .fromBytes(table.getActiveTimeline().getInstantDetails(lastInstant
+                .get()).get());
+        rollingStatMetadata = rollingStatMetadata


if a compaction runs and thus the log becomes empty now, don't we have to reset the stats? We always seem to be merging with previous stats?

well, the stats are the same irrespective of whether it's log files or parquet files. Resetting the stats could be a good size optimization since the stats will grow over time. It is tricky to erase the stats based on compaction since we have parallel compactions running. For example, say we look at the timeline and get all the compaction commits present after the last deltacommit, then we have to look up all the fileId's for all those compaction commits, if there was no new log file written for that file id while that compaction took place, then reset the stats, else keep the stats since there is a phantom parquet for which new log files were created. There are other corner cases too which I cannot think of now (but had considered this approach when I first opened the PR). If you strongly feel we should invest in the space optimization aspect, I can spend time digging deeper into this to list out the issues. Let me know.

vinothchandar · 2018-06-26T17:12:22Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java

+                // --------------------------------------------------------------------------------------------------
+                // (A) The following cases are possible if index.canIndexLogFiles and/or index.isGlobal
+                // --------------------------------------------------------------------------------------------------
+                // (A.1) Failed first commit - Inserts were written to log files and HoodieWriteStat has no entries. In


deleting logs is bit different from deleting parquet files.. Queries can pick the log file up and may crash in between if we delete them?

In case of parquet files, they are not published to the query, so deleting works.

These are deleting log files for failed first commits. The queries should not pick up failed commits.

vinothchandar · 2018-06-26T17:31:58Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java

+        if (!index.canIndexLogFiles()) {
+          // TODO : choose last N small files since there can be multiple small files written to a single partitions
+          // in a single batch
+          Optional<FileSlice> smallFile = getRTFileSystemView()


rename: smallFileSlice

vinothchandar · 2018-06-26T17:32:36Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java

+    }
+
+    // TODO (NA) : Make this static part of utility
+    protected long convertLogFilesSizeToExpectedParquetSize(List<HoodieLogFile> hoodieLogFiles) {


add some tests for this?

I've added some extra cases to testUpsertPartitioner already which should cover this but I agree we should have more granular unit test here. Let me think about this.

n3nash · 2018-06-28T23:38:21Z

@vinothchandar @bvaradar addressed comments, left a couple of comments on 2 concerns. Please take a pass.

vinothchandar

@n3nash looks like only thing remaining is the extra test case?

bvaradar · 2018-07-17T00:55:44Z

@n3nash @vinothchandar : As we have been testing Async Compaction in staging without this diff for sometime now, Can we wait for async compaction diff to land first before we merge this diff. It also helps me avoid doing numerous rebases to make the Async Compaction Stacked diffs consistent.

Balaji.V

n3nash · 2018-07-17T17:52:53Z

@bvaradar I'm fine waiting till your diff gets landed.

vinothchandar · 2018-08-31T14:27:32Z

@n3nash can you rebase this one?

…he total size of the log file is compared with the parquet max file size and if there is scope to add inserts the add it.

n3nash · 2018-09-04T08:58:30Z

@vinothchandar rebased, please take a pass on HoodieMergeOnReadTable and HoodieAppendHandle since I've made a few new changes there.

vinothchandar · 2018-09-04T21:50:23Z

@n3nash can you briefly describe your delta, HoodieMergeOnReadTable is pretty big.

n3nash · 2018-09-05T18:39:38Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java


                  // read commit file and (either append delete blocks or delete file)
                  final Map<FileStatus, Boolean> filesToDeletedStatus = new HashMap<>();
                  Map<FileStatus, Long> filesToNumBlocksRollback = new HashMap<>();

                  // In case all data was inserts and the commit failed, delete the file belonging to that commit
+                  // We do not know fileIds for inserts (first inserts are either log files or parquet files),


@vinothchandar Changes in comments here

n3nash · 2018-09-05T18:40:34Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java

+
+  private HoodieRollbackStat rollback(HoodieIndex hoodieIndex, String partitionPath, String commit,
+      HoodieCommitMetadata commitMetadata, final Map<FileStatus, Boolean> filesToDeletedStatus,
+      Map<FileStatus, Long> filesToNumBlocksRollback, Set<String> deletedFiles) {


Pass the deletedFiles as an argument here @vinothchandar

n3nash · 2018-09-05T18:41:18Z

@vinothchandar Tagged you in the places where changes have happened

n3nash · 2018-09-05T18:42:36Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java

+        .filter(wStat -> {
+          // Filter out stats without prevCommit since they are all inserts
+          if (wStat != null && wStat.getPrevCommit() != HoodieWriteStat.NULL_COMMIT && wStat.getPrevCommit() != null
+              && !deletedFiles.contains(wStat.getFileId())) {


Check deletedFiles here @vinothchandar

vinothchandar · 2018-09-06T00:50:59Z

hoodie-client/src/main/java/com/uber/hoodie/table/HoodieMergeOnReadTable.java

+  private HoodieRollbackStat rollback(HoodieIndex hoodieIndex, String partitionPath, String commit,
+      HoodieCommitMetadata commitMetadata, final Map<FileStatus, Boolean> filesToDeletedStatus,
+      Map<FileStatus, Long> filesToNumBlocksRollback, Set<String> deletedFiles) {
+    // The following needs to be done since GlobalIndex at the moment does not store the latest commit time.


@n3nash can you compile a list of these one-offs into a ticket.. we need to fish them all out eventually.

vinothchandar · 2018-09-06T00:52:03Z

thanks.. Assuming you have already thoroughly tested this, looks okay to me.

Co-authored-by: Lokesh Lingarajan <lokeshlingarajan@Lokeshs-MacBook-Pro.local>

This was referenced Jun 12, 2018

(WIP) Small file corrections with inserts into log files #396

Closed

(WIP) Small file corrections with inserts log files #402

Closed

n3nash force-pushed the small_file_handling_with_inserts_into_logfile branch from 32405eb to 8a2bdbf Compare June 12, 2018 18:21

bvaradar reviewed Jun 25, 2018

View reviewed changes

vinothchandar requested changes Jun 26, 2018

View reviewed changes

n3nash force-pushed the small_file_handling_with_inserts_into_logfile branch from 8a2bdbf to d058e5b Compare June 26, 2018 23:38

vinothchandar approved these changes Jul 16, 2018

View reviewed changes

bvaradar mentioned this pull request Aug 2, 2018

FileSystemView and Timeline level changes to support Async Compaction #390

Closed

n3nash force-pushed the small_file_handling_with_inserts_into_logfile branch from d058e5b to 7eb9ace Compare September 2, 2018 07:04

1. Small file size handling for inserts into log files. In summary, t…

704bd86

…he total size of the log file is compared with the parquet max file size and if there is scope to add inserts the add it.

n3nash force-pushed the small_file_handling_with_inserts_into_logfile branch from 7eb9ace to 704bd86 Compare September 4, 2018 07:48

n3nash commented Sep 5, 2018

View reviewed changes

vinothchandar reviewed Sep 6, 2018

View reviewed changes

vinothchandar merged commit 459e523 into apache:master Sep 6, 2018

vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023

[AUDIT-376] - Upgrade to version release-v0.26.0 (apache#413)

89f1d2b

Co-authored-by: Lokesh Lingarajan <lokeshlingarajan@Lokeshs-MacBook-Pro.local>

Small file size handling for inserts into log files. #413

Small file size handling for inserts into log files. #413

Conversation

n3nash commented Jun 12, 2018 • edited

n3nash commented Jun 12, 2018

bvaradar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3nash commented Jun 28, 2018

vinothchandar left a comment

Choose a reason for hiding this comment

bvaradar commented Jul 17, 2018

n3nash commented Jul 17, 2018

vinothchandar commented Aug 31, 2018

n3nash commented Sep 4, 2018

vinothchandar commented Sep 4, 2018

Choose a reason for hiding this comment

n3nash Sep 5, 2018 • edited

Choose a reason for hiding this comment

n3nash commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar commented Sep 6, 2018

n3nash commented Jun 12, 2018 •

edited

n3nash Sep 5, 2018 •

edited