grantatspothero commented on code in PR #18076:
URL: https://github.com/apache/iceberg/pull/18076#discussion_r4008780357
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -193,34 +200,54 @@ private ManifestFile createManifest(int specId,
List<ManifestFile> bin) {
}
ManifestWriter<F> writer = newManifestWriter(spec(specId));
+ ExecutorService workerPool = workerPoolSupplier.get();
+ Deque<FutureTask<List<ManifestEntry<F>>>> pendingReads = new
ArrayDeque<>();
boolean threw = true;
try {
- for (ManifestFile manifest : bin) {
- boolean isCommitted =
- manifest.snapshotId() != null && snapshotId() !=
manifest.snapshotId();
- try (ManifestReader<F> reader = newManifestReader(manifest,
isCommitted)) {
- for (ManifestEntry<F> entry : reader.entries()) {
- if (entry.status() == Status.DELETED) {
- // suppress deletes from previous snapshots. only files deleted
by this snapshot
- // should be added to the new manifest
- if (entry.snapshotId() == snapshotId()) {
- writer.delete(entry);
- }
- } else if (entry.status() == Status.ADDED && entry.snapshotId() ==
snapshotId()) {
- // adds from this snapshot are still adds, otherwise they should
be existing
- writer.add(entry);
- } else {
- // add all files from the old manifest as existing files
- writer.existing(entry);
- }
+ // reads run on the worker pool ahead of the writer, which consumes them
in bin order. a
Review Comment:
Makes sense re: ParallelIterable deadlock and ordering.
The code is quite complicated due to all of the above constraints and the
constraint to tightly bound memory within a bin. Do we need to tightly bound
memory usage within a bin? Notably each bin is roughly bound to on disk
representation of `MANIFEST_TARGET_SIZE_BYTES_DEFAULT = 8MB`. Assuming 10x/100x
larger heap size, that is 80MB/800MB worst case per bin.
The common case is only 1 large bin needs to be rewritten. EG) 100 commits,
100 small manifests, 1 bin gets rewritten.
I'm concerned `MAX_READ_AHEAD_ENTRIES` adds significant complexity for
modest benefit over the naive approach. For example, each ManifestEntry can
vary in size so even this read ahead bound is not tight. Would be good to get
others feedback.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
