Core: Avoid generating a large ManifestFile when committing by ConeyLiu · Pull Request #6335 · apache/iceberg

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.

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.

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

  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.

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

+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

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

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

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

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

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

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

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

+  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

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


Optional: committedNewManifests -> committedNewDataManifests?

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

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

…ef. apache#6335)

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

amogh-jahagirdar reviewed Dec 5, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/SnapshotProducer.java Outdated

Comment thread core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Outdated

Comment thread core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Outdated

rdblue reviewed Dec 18, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Outdated

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