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: 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`. Even assuming 10x larger heap size, that is 80MB per bin.
And the common case is only 1 bin needs to be rewritten. EG) 100 commits,
100 small manifests, 1 bin gets rewritten. You can construct an uncommon case
where there are many bins but it would require a table with many small
manifests that was never compacted.
I'm concerned `MAX_READ_AHEAD_ENTRIES` adds significant complexity for not
much benefit over the naive approach. 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]
