Core: Avoid generating a large ManifestFile when committing #6335

ConeyLiu · 2022-12-01T13:45:51Z

In our production env, we noticed the manifest files have a large random size, ranging from several KB to larger than 100 MB. It seems the MANIFEST_TARGET_SIZE_BYTES has not worked during the commit phase.

In this PR, we avoid generating a manifest file larger than MANIFEST_TARGET_SIZE_BYTES for newly added content files and will generate multiple manifest files when the size is covered.

ConeyLiu · 2022-12-01T13:47:40Z

Hi @szehon-ho @rdblue @pvary @stevenzwu @chenjunjiedada pls help to review this when you are free. Thanks a lot.

amogh-jahagirdar

Thanks @ConeyLiu! just had some questions/comments on this, but this behavior is something I observed as well.

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

nastra · 2022-12-05T15:16:54Z

It is mentioned in the docs that MANIFEST_TARGET_SIZE_BYTES relates to Target size when merging manifest files, meaning that this setting only takes effect when merging of manifest files happens (e.g. when using newAppend()). Merging of manifest files would not happen when using newFastAppend() for example. This might explain why it seemed that this setting wouldn't take any effect in your env.

ConeyLiu · 2022-12-06T13:45:26Z

@nastra newAppend will create a large manifest as well. It only merges small manifests into a large one, and does not split large one into target file size.

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

rdblue · 2022-12-18T23:43:18Z

This seems like a reasonable thing to add to me. I'm actually more concerned about the use case that caused this.

@ConeyLiu, what was the case where you were creating huge manifests? How many data files were committed?

ConeyLiu · 2022-12-19T07:21:17Z

@rdblue thanks for the reviewing. It happens for the larger table (has several PBs data) and with many columns(thousand columns which is very common for log data or feature data). And the size of the DataFile metrics' is increased with the number of columns. So the manifest file could be really large when the user adds into/overwrites a larger number of data (eg, sync hourly data).

RussellSpitzer

I see two sets of changes here, one around "caching" and one around rolling over manifest files. Can we separate out the changes?

ConeyLiu · 2023-01-07T08:42:25Z

core/src/main/java/org/apache/iceberg/FastAppend.java

@@ -49,8 +51,9 @@ class FastAppend extends SnapshotProducer<AppendFiles> implements AppendFiles {
  private final List<DataFile> newFiles = Lists.newArrayList();
  private final List<ManifestFile> appendManifests = Lists.newArrayList();
  private final List<ManifestFile> rewrittenAppendManifests = Lists.newArrayList();
-  private ManifestFile newManifest = null;
+  private List<ManifestFile> cachedNewManifests = null;


@RussellSpitzer Do you mean the caching part here? Here just rename the variable because it is plural now.

ConeyLiu · 2023-05-12T07:09:42Z

@rdblue @RussellSpitzer @nastra @amogh-jahagirdar Would you mind taking another look at this when you are free? Thanks a lot.

zinking · 2023-06-01T06:26:29Z

@rdblue @RussellSpitzer @szehon-ho can we have another look at this review and merge if possible ? thanks. large manifests have huge performance penalties for PetaByte tables.

szehon-ho · 2023-06-29T22:25:13Z

Yea sorry let me try to review this tomorrow.

core/src/main/java/org/apache/iceberg/FastAppend.java

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

ConeyLiu · 2023-07-07T15:15:52Z

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/** As opposed to {@link ManifestWriter}, a rolling writer could produce multiple manifest file. */
+abstract class RollingManifestWriter<T extends ContentFile<T>> implements FileAppender<T> {


@szehon-ho pls take a look if this is the right direction. Here implements FileAppender instead of FileWriter that's because the ManifestWriter implements FileAppender.

Thanks, this definitely looks cleaner.

Im just not sure what to do with existing hierarchy (ManifestWriter), as there are many duplications.

Did we think about modifying ManifestWriter, to take extra variable 'targetFileSizeBytes', then trigger tryRollover only in this case.

The problem may be the method manifestFile() in writer? It should have been manifestFiles(). Maybe we can deprecate existing method?

CC @aokolnychyi .

Actually on second read, this may be ok, as its a composition, and not duplicating that much code. Lets see if Anton has any comments

core/src/main/java/org/apache/iceberg/FastAppend.java

szehon-ho · 2023-07-10T23:02:50Z

core/src/main/java/org/apache/iceberg/FastAppend.java

+      //   // Operation succeeds and calls cleanUncommitted'
+      //   txn.newFastAppend().appendFile(...).commit();
+      //   // Some other operations ...
+      //   // Commit fails and needs to clean up newManifests


Nit: should this comment be after next one? (seems comments refer to the line above)

I add the blank line between the comments to separate them.

core/src/main/java/org/apache/iceberg/FastAppend.java

core/src/main/java/org/apache/iceberg/ManifestFiles.java

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

szehon-ho · 2023-07-11T01:09:40Z

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/** As opposed to {@link ManifestWriter}, a rolling writer could produce multiple manifest file. */
+abstract class RollingManifestWriter<T extends ContentFile<T>> implements FileAppender<T> {


Thanks, this definitely looks cleaner.

Im just not sure what to do with existing hierarchy (ManifestWriter), as there are many duplications.

Did we think about modifying ManifestWriter, to take extra variable 'targetFileSizeBytes', then trigger tryRollover only in this case.

The problem may be the method manifestFile() in writer? It should have been manifestFiles(). Maybe we can deprecate existing method?

CC @aokolnychyi .

aokolnychyi · 2023-07-26T04:35:37Z

I will take a look with fresh eyes by the end of tomorrow.

core/src/main/java/org/apache/iceberg/ManifestOutputFileFactory.java

core/src/main/java/org/apache/iceberg/ManifestFiles.java

aokolnychyi · 2023-07-27T06:03:30Z

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

+  private void tryRollingToNewFile() {
+    currentFileRows++;
+
+    if (currentWriter.length() > targetFileSizeInBytes) {


Will it be expensive to call for every file? In rolling file writers, we check only every 1000 records. I understand that value may be too big but I still don't think it is a good idea to check the length for every entry.

How about checking every 16? Just simply calculated as: 1000 / (512 / 8).

Could you check the average cost of an entry in a manifest in some of your real tables? My assumption was something around 250 but let's check the average size of an entry.

I have checked some tables. The average size has a lot to do with the schema of the table, the more columns take up more storage space. That's because of the column metrics. I noticed the biggest one (with thousand columns) needs about 5KB for one entry. The small tables (about 10 columns) need about several hundred bytes. So 250 should be an acceptable number.

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

core/src/main/java/org/apache/iceberg/FastAppend.java

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

core/src/main/java/org/apache/iceberg/FastAppend.java

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

aokolnychyi · 2023-07-27T21:44:48Z

I feel like this would be super valuable for CTAS kind of use cases where a large chunk of data is being added at the same time. I had a few suggestions on how to simplify the rolling writer.

Nice work, @ConeyLiu!

ConeyLiu · 2023-07-28T10:02:24Z

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

+  private boolean closed = false;
+
+  public RollingManifestWriter(
+      FileIO fileIO,


Used to delete the empty file.

Do you think it is still needed? We switched to a lazy writer that is initialized only when there is a new entry to write. The same also applies to cases when we roll to a new file. It can only happen if there is a pending entry to write. If I am not missing anything, we can drop FileIO here and simplify closeCurrentWriter.

Sorry my bad, the empty file checking is not necessary.

aokolnychyi

This change looks good to me. I had one question. @ConeyLiu, could you check if I missed something?

aokolnychyi · 2023-07-28T16:08:15Z

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

-      deleteFile(cachedNewDataManifest.path());
-      this.cachedNewDataManifest = null;
+    if (cachedNewDataManifests != null) {
+      List<ManifestFile> committedNewManifests = Lists.newArrayList();


Optional: committedNewManifests -> committedNewDataManifests?

aokolnychyi · 2023-07-28T16:13:22Z

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java

+  private boolean closed = false;
+
+  public RollingManifestWriter(
+      FileIO fileIO,


Do you think it is still needed? We switched to a lazy writer that is initialized only when there is a new entry to write. The same also applies to cases when we roll to a new file. It can only happen if there is a pending entry to write. If I am not missing anything, we can drop FileIO here and simplify closeCurrentWriter.

ConeyLiu · 2023-07-28T16:54:15Z

@aokolnychyi thanks for the details reviewing. The empty file checking is not necessary. Reverted related code changes.

aokolnychyi · 2023-07-28T18:06:45Z

Thanks, @ConeyLiu! Thanks everyone for reviewing!

ConeyLiu · 2023-07-29T01:41:05Z

Thanks @aokolnychyi for mering this. Thanks @rdblue @RussellSpitzer @szehon-ho @amogh-jahagirdar for reviewing.

github-actions bot added the core label Dec 1, 2022

amogh-jahagirdar reviewed Dec 5, 2022

View reviewed changes

rdblue reviewed Dec 18, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Outdated Show resolved Hide resolved

ConeyLiu force-pushed the files-as-multiple-mf branch from 3307397 to 6b21694 Compare December 19, 2022 07:06

ConeyLiu force-pushed the files-as-multiple-mf branch from 6b21694 to 85c05cc Compare December 24, 2022 12:14

RussellSpitzer reviewed Jan 6, 2023

View reviewed changes

ConeyLiu commented Jan 7, 2023

View reviewed changes

ConeyLiu force-pushed the files-as-multiple-mf branch from 85c05cc to 7ac0847 Compare May 12, 2023 07:03

ConeyLiu force-pushed the files-as-multiple-mf branch from d1e1f1d to 617647e Compare June 2, 2023 09:53

szehon-ho reviewed Jul 1, 2023

View reviewed changes

ConeyLiu force-pushed the files-as-multiple-mf branch from 9074d09 to e27ba9b Compare July 7, 2023 15:13

ConeyLiu commented Jul 7, 2023

View reviewed changes

szehon-ho reviewed Jul 11, 2023

View reviewed changes

aokolnychyi reviewed Jul 27, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/ManifestOutputFileFactory.java Outdated Show resolved Hide resolved

aokolnychyi reviewed Jul 27, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/ManifestFiles.java Outdated Show resolved Hide resolved

aokolnychyi reviewed Jul 27, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java Outdated Show resolved Hide resolved

aokolnychyi reviewed Jul 27, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/RollingManifestWriter.java Outdated Show resolved Hide resolved

aokolnychyi reviewed Jul 27, 2023

View reviewed changes

avoid generating large manifest file

d83ad26

ConeyLiu force-pushed the files-as-multiple-mf branch from 966a7f8 to d83ad26 Compare July 28, 2023 09:32

ConeyLiu commented Jul 28, 2023

View reviewed changes

aokolnychyi reviewed Jul 28, 2023

View reviewed changes

address comments

73d4abb

aokolnychyi approved these changes Jul 28, 2023

View reviewed changes

aokolnychyi merged commit a7a09d4 into apache:master Jul 28, 2023
41 checks passed

ConeyLiu deleted the files-as-multiple-mf branch July 29, 2023 01:41

This was referenced Dec 5, 2023

Fixed certain operations failing to add new data files during retries #9222

Closed

Manually retrying commits can result in partially applied changes #9227

Closed

Core: Fixed certain operations failing to add new data files during retries #9230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Avoid generating a large ManifestFile when committing #6335

Core: Avoid generating a large ManifestFile when committing #6335

ConeyLiu commented Dec 1, 2022

ConeyLiu commented Dec 1, 2022

amogh-jahagirdar left a comment

nastra commented Dec 5, 2022

ConeyLiu commented Dec 6, 2022

rdblue commented Dec 18, 2022

ConeyLiu commented Dec 19, 2022

RussellSpitzer left a comment

ConeyLiu Jan 7, 2023

ConeyLiu commented May 12, 2023

zinking commented Jun 1, 2023 •

edited

szehon-ho commented Jun 29, 2023

ConeyLiu Jul 7, 2023

szehon-ho Jul 11, 2023

szehon-ho Jul 11, 2023

szehon-ho Jul 10, 2023

ConeyLiu Jul 12, 2023

szehon-ho Jul 11, 2023

aokolnychyi commented Jul 26, 2023

aokolnychyi Jul 27, 2023

ConeyLiu Jul 27, 2023

aokolnychyi Jul 27, 2023

ConeyLiu Jul 28, 2023

aokolnychyi commented Jul 27, 2023

ConeyLiu Jul 28, 2023

aokolnychyi Jul 28, 2023

ConeyLiu Jul 28, 2023

aokolnychyi left a comment

aokolnychyi Jul 28, 2023

ConeyLiu Jul 28, 2023

aokolnychyi Jul 28, 2023

ConeyLiu commented Jul 28, 2023

aokolnychyi commented Jul 28, 2023

ConeyLiu commented Jul 29, 2023

Core: Avoid generating a large ManifestFile when committing #6335

Core: Avoid generating a large ManifestFile when committing #6335

Conversation

ConeyLiu commented Dec 1, 2022

ConeyLiu commented Dec 1, 2022

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

nastra commented Dec 5, 2022

ConeyLiu commented Dec 6, 2022

rdblue commented Dec 18, 2022

ConeyLiu commented Dec 19, 2022

RussellSpitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConeyLiu commented May 12, 2023

zinking commented Jun 1, 2023 • edited

szehon-ho commented Jun 29, 2023

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

aokolnychyi commented Jul 26, 2023

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

aokolnychyi commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi 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

ConeyLiu commented Jul 28, 2023

aokolnychyi commented Jul 28, 2023

ConeyLiu commented Jul 29, 2023

zinking commented Jun 1, 2023 •

edited