You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/04/20 12:35:35 UTC
[GitHub] [iceberg] uncleGen opened a new pull request, #4596: Use bounded queue to avoid consuming too much memory
uncleGen opened a new pull request, #4596:
URL: https://github.com/apache/iceberg/pull/4596
In my scene, there are hundreds of thousands of datafiles. If `iceberg.scan.plan-in-worker-pool` is enabled, OOM exception happend continually. The root cause is an unbounded queue `ConcurrentLinkedQueue` is used in `ParallelIterator`. The queue will consume much memory.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1139231560
@rdblue We implemented file-level index for iceberg and use the index to determine whether a file satisfies the query predicate. Although the index file is much smaller than the data file, we wouldn't want to load them in trino coordinator. So the filtering happens on worker nodes. Trino coordinator schedules the file scan tasks in a streaming fashion, so it doesn't have to hold all of them in memory (it does such thing during query optimization though, which we are trying to avoid).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138139258
@szehon-ho Do you have an example in mind where blocking queue can hurt performance? IMHO, if a blocking queue blocks the producers, it usually means the consumer is not fast enough, in which case the bottleneck is the consumer, rather than the queue itself.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1108116245
@rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1168430308
@rdblue The issue I mentioned is only critical with blocking queue. Because the producer thread can block forever and if the pool is full of such threads, no new tasks can be run. If we have reached consensus to use blocking queue, I can submit a PR for it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1137635703
@rdblue @lirui-apache thanks for confirming, so I think using 'rewriteManifest' and specifying the commit.manifest.target-size-byte to a reasonable size (so you have an even number of entries per manifest file), along with a system property: 'iceberg.worker.num-threads' to control how many manifests are read at once. I wonder if that will help that memory problem?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
hililiwei commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1144847321
Agree with @openinx. also +1 to the bound queue solution. I was in the same predicament.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r855022883
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +39,25 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
- static boolean getBoolean(String systemProperty, boolean defaultValue) {
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
+
+ public static boolean getBoolean(String systemProperty, boolean defaultValue) {
Review Comment:
This method can still be package private
##########
core/src/main/java/org/apache/iceberg/util/ParallelIterable.java:
##########
@@ -55,7 +55,9 @@ public CloseableIterator<T> iterator() {
private final Iterator<Runnable> tasks;
private final ExecutorService workerPool;
private final Future<?>[] taskFutures;
- private final ConcurrentLinkedQueue<T> queue = new ConcurrentLinkedQueue<>();
+ private final int queueSize = SystemProperties.getInt(SystemProperties.SCAN_SHARED_QUEUE_SIZE,
+ SystemProperties.SCAN_SHARED_QUEUE_SIZE_DEFAULT);
+ private final LinkedBlockingQueue<T> queue = new LinkedBlockingQueue<>(queueSize);
Review Comment:
Can these be initialised in the constructor?
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +39,25 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
- static boolean getBoolean(String systemProperty, boolean defaultValue) {
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
+
+ public static boolean getBoolean(String systemProperty, boolean defaultValue) {
String value = System.getProperty(systemProperty);
if (value != null) {
return Boolean.parseBoolean(value);
}
return defaultValue;
}
+
+ public static int getInt(String systemProperty, int defaultValue) {
+ String value = System.getProperty(systemProperty);
Review Comment:
As this method is public static so we should check if `systemProperty` is not null.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r855885750
##########
core/src/main/java/org/apache/iceberg/util/ParallelIterable.java:
##########
@@ -65,10 +67,10 @@ private ParallelIterator(Iterable<? extends Iterable<T>> iterables,
try (Closeable ignored = (iterable instanceof Closeable) ?
(Closeable) iterable : () -> { }) {
for (T item : iterable) {
- queue.add(item);
+ queue.put(item);
}
- } catch (IOException e) {
- throw new RuntimeIOException(e, "Failed to close iterable");
+ } catch (IOException | InterruptedException e) {
+ throw new RuntimeException("Failed to close iterable", e);
Review Comment:
`InterruptedException` is thrown by `BlockingQueue.put()`. Using `put()` but not `add()` is because `put()` will , waiting if necessary for space to become available.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen closed pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen closed pull request #4596: Use bounded queue to avoid consuming too much memory
URL: https://github.com/apache/iceberg/pull/4596
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r855885750
##########
core/src/main/java/org/apache/iceberg/util/ParallelIterable.java:
##########
@@ -65,10 +67,10 @@ private ParallelIterator(Iterable<? extends Iterable<T>> iterables,
try (Closeable ignored = (iterable instanceof Closeable) ?
(Closeable) iterable : () -> { }) {
for (T item : iterable) {
- queue.add(item);
+ queue.put(item);
}
- } catch (IOException e) {
- throw new RuntimeIOException(e, "Failed to close iterable");
+ } catch (IOException | InterruptedException e) {
+ throw new RuntimeException("Failed to close iterable", e);
Review Comment:
`InterruptedException` is thrown by `BlockingQueue.put()`. Using `put()` but not `add()` is because `put()` will waiting if necessary for space to become available.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138704180
@lirui-apache, the files should be added to the queue after being filtered. Won't you need to hold all these files in memory at some point anyway? Or are you avoiding that somehow?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r904514624
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
+
static boolean getBoolean(String systemProperty, boolean defaultValue) {
String value = System.getProperty(systemProperty);
if (value != null) {
return Boolean.parseBoolean(value);
}
return defaultValue;
}
+
+ public static int getInt(String systemProperty, int defaultValue) {
+ Preconditions.checkNotNull(systemProperty, "System property name should not be null");
Review Comment:
General but / FYI - we almost always tend to prefer plain English language with a structure similar to “general type of problem / problem phrase [possible solution only if not clear from stack trace]: offending value”.
we put the bad value at the end after a colon to give us a more continuous search string for logs (instead of mixing them into the sentence).
So for this situation, the suggested sentence would be format(“Invalid value for system property %s: null”, systemProperty);
However for this we can be more specific as null here means missing / not set almost always (and anybody who set it to null will see that). So I’d suggest format(“Invalid value for system property %s: null”, systemProperty); The final `: null` is debatable but I’d personally put it just to cover any cases where it might be explicitly null and just to match many others preconditions.
Yours is pretty good, but this tends to be our standard. I think it’s most helpful thinking of it in the context of the NPE or IllegalArumentException and full stack trace. What’s invariably helpful is having a common-ish way of writing these with a long enough, specific enough stack trace to be searchable, with less and less specific search query as the phrase is shortened.
Hope that helps for the long term! It really is just a nit but if you’re working on Iceberg offen enough it might help you as a contributor and a user.
```suggestion
Preconditions.checkNotNull(systemProperty, String.format("Invalid value for system property %s: null", systemProperty);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r904514624
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
+
static boolean getBoolean(String systemProperty, boolean defaultValue) {
String value = System.getProperty(systemProperty);
if (value != null) {
return Boolean.parseBoolean(value);
}
return defaultValue;
}
+
+ public static int getInt(String systemProperty, int defaultValue) {
+ Preconditions.checkNotNull(systemProperty, "System property name should not be null");
Review Comment:
General but / FYI - we almost always tend to prefer plain English language with a structure similar to “general type of problem / problem phrase [possible solution only if not clear from stack trace]: offending value”.
we put the bad value at the end after a colon to give us a more continuous search string for logs (instead of mixing them into the sentence).
So for this situation, the suggested sentence would be format(“Invalid value for %s: null”, systemProperty);
However for this we can be more specific as null here means missing / not set almost always (and anybody who set it to null will see that). So I’d suggest format(“Missing system property for %s: null”, systemProperty); The final `: null` is debatable but I’d personally put it just to cover any cases where it might be explicitly null and just to match many others preconditions.
Yours is pretty good, but this tends to be our standard. I think it’s most helpful thinking of it in the context of the NPE or IllegalArumentException and full stack trace. What’s invariably helpful is having a common-ish way of writing these with a long enough, specific enough stack trace to be searchable, with less and less specific search query as the phrase is shortened.
Hope that helps for the long term! It really is just a nit but if you’re working on Iceberg offen enough it might help you as a contributor and a user.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1164188977
I also want to mention that if `close` and `hasNext` in ParallelIterator are called concurrently, there might be thread safety issues, e.g. new tasks submitted during close can be leaked in the pool. I think it's safer to make the producer thread periodically check the `closed` flag, rather than block on the queue forever.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1136529127
Took a little a look through the code,
ParallelIterable.hasNext() calls [checkTasks() ](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/util/ParallelIterable.java#L97) , which then calls submitNextTask() but only up to taskFuture size (configured thread pool size).
[submitNextTask](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/util/ParallelIterable.java#L129) is the one that calls tasks.next(), which adds things to the queue. I guess that's why @rdblue is mentioning, it is self-limiting.
The problem may be that each [tasks.next()](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/util/ParallelIterable.java#L65) does dump the entire nested Iterable into the queue.
So in real life example of the planning case, it seems the level parallelism is manifest. New manifests may be blocked to be processed if nobody is consuming, but a single or few manifests having a lot of entries may cause memory pressure as they dump all of the entries. @lirui-apache @uncleGen is that the case? Probably the fix is to rewriteManifests to have more reasonable number of entries?
I guess if we use blocking queue, it may be useful and we have another knob to limit the memory, but then it's not truly parallel by manifest, ie some manifests may be blocked a long time before they can submit their entries to the queue. @rdblue let me know if you have any thoughts or if I may have misread the code.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138884615
> @kbendick
>
> > Is there a reason that the files have to be 1MB?
>
> There is a streaming ingest job, it will create much small files.
>
> > But I think if you compacted files you probably wouldn’t have such issues during scan planning.
> > Do you run table maintenance actions on your table?
>
> I have run `rewrite-files` action once a day. Ideally, reducing the interval may bypass this issue. But bypassing this issue requires carefully considering jvm heap configuration with existing small files.
Even for streaming ingest, there are ways to make the small files problem better. For example, if you're ingesting with Flink, you can increase the time between commits. Or you can also play with `write.distribution.mode` to try to send data to the correct writer before generating the files. Which might reduce the overall number of files you have.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r855666619
##########
core/src/main/java/org/apache/iceberg/DeleteFileIndex.java:
##########
@@ -377,7 +377,7 @@ Builder planWith(ExecutorService newExecutorService) {
}
DeleteFileIndex build() {
- // read all of the matching delete manifests in parallel and accumulate the matching files in a queue
+ // read all the matching delete manifests in parallel and accumulate the matching files in a queue
Review Comment:
Nit: Can we avoid making unnecessary changes to files?
Every additional unneeded diff makes the work of cherry-picking individual commits into private forks that much more difficult for those who maintain their own company-specific forks.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r857185569
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
Review Comment:
This seems really small if we're trying to avoid running out of memory. I think it is likely that this will cause a performance bottleneck.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1125706133
ping @rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138888793
Also, for compacting small files (as well as avoiding small files in ingest), I just wrote a somewhat in depth summary in this issue that might be relevant to your streaming ingest (and data file rewrite operations): https://github.com/apache/iceberg/issues/4875
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138764553
Yea , good idea to adding plan parallelism per table if not there already (it might be in Flink but not Spark)
Yea, also curious, in Spark it seems the FileScanTask iterator in the end gets into a concrete array, though I guess its possible to consume the FileScanTask directly in some cases as soon as it's read.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138096307
Let me clarify our use case. We have an iceberg table partitioned by date, and we run an ETL job every day to sync data from an upstream hive table into this iceberg table. The ETL job basically just runs an `INSERT INTO` with SparkSQL which adds a new partition to the iceberg table. So we end up having a manifest for each partition, and each partition has lots of data files, i.e. ranging roughly from 50k to 130k.
Then we have a trino cluster where users submit ad-hoc queries. This cluster is multi-tenant and not just meant for the iceberg table mentioned above.
The problem we faced was that querying this huge iceberg table can easily make the trino coordinator unstable or even crash with OOM.
I'll check whether `commit.manifest.target-size-byte` can mitigate our case. But IIUC, the iceberg worker pool is static and shared among all jobs. So we probably won't want to change the pool size.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1148172108
I do see the points you raise and have admittedly used a BlockingQueue in similar situations in large scale streaming ETL in the past (thinking in terms of manifests in the sort of “envelope” sense that is manifest lists and even just overall snapshot change set).
Can we introduce a configuration parameter, with a blocking queue size? If we give the parameter a negative value (just like `CachingCatalog`s cache timeout milliseconds disables the cache when negative)? This way the user can avoid the queue or keep old behavior but also users interested in trying the BlockingQueue approach can do so (as it has served me well, particularly in streaming scenario where the pipeline cannot stop).
This is also not unlike `streamResults` parameter for some spark driver side operations or akin to whether or not the worker thread pool is used in my opinion. So if we configure it in terms of size of queue, that’s the trade off users can more easily tune.
Would this be achievable?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1148158612
> IMHO, if a blocking queue blocks the producers, it usually means the consumer is not fast enough, in which case the bottleneck is the consumer, rather than the queue itself.
> Does the blocking queue approach introduce any substantial performance bottleneck ? If we think the default blocking queue size is a bit small, then we can increase this default blocking queue size to 10000.
IIUC, `blocking queue` should not introduce any bottleneck. Besides, if `blocking queue` can resolve OOM issue, it will be better and easier than doing additional tuning works for dev
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1165716433
@lirui-apache, do you want to open a PR for that fix?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1163915875
> Let me be clear. In my scene, my machine has 96 cores, when `iceberg.scan.plan-in-worker-pool` is enabled, the `iceberg.worker.num-threads` will be number of available processors in default, i.e. 96. So the input speed of queue is far greater than outpu speed of queue. Then OOM occurs.
Forgive me if I missed this as I’m just coming back to it, but what happens if you lower the core count in iceberg.sc.plan-in-worker-pool via iceberg.worker.num-threads?
I’ve heard of other issues with machines with very large number of cores.
Maybe, possibly in addition to this, we might consider a configurable max limit and default it to something sane? The other report was from a Spark driver iirc.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1163942570
Also @uncleGen,
You might be interested in these two PRs which might help your use case (increase output speed of queue): - https://github.com/apache/iceberg/pull/4911 (merged in master and will be released in 0.14.0)
- https://github.com/apache/iceberg/pull/4943 (yet to be merged but will also help this situation generally speaking)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138639623
@lirui-apache, another option here is to add the ability to set parallelism on a per-table basis. Basically to prevent the parallel iterator from submitting so many tasks at once. That may help your situation.
That said, I still think the main problem is that your partitions have 50-130k files. Have you tried compacting those?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1138691153
@rdblue Thanks for the advice. Actually it's not a small-file problem. Each partition has over 200 billion records. We do have optimizations to make sure each query only scans a small portion. But it cannot help if we hit OOM at the planning phase.
I also noticed latest code supports planning files with separate pools. So with separate pools + limited manifest entries + limited pool size, I think we can bring the memory usage under control. Although personally I still prefer the blocking queue solution, which seems easier to achieve and more reliable.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1151064824
> Can we introduce a configuration parameter, with a blocking queue size? If we give the parameter a negative value, ideally the user would not use a queue but would keep current behavior
+1, this is exactly how we implemented internally.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1108116675
@rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1108116420
@rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r904525876
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
Review Comment:
The worker pool is already used for a number of things though and is presently uncapped.
Going from unbounded to bounded is already a potential drain / possible source of bottleneck. I don't know what the right value is for this, but if we're considering this feature, I would say this is the best value to use as a configuration to disable it.
There could be generally other scans taking place and many file scan tasks are unfortunately somewhat variable in size and can be combined etc. So I don't disagree with using the larger value (especially if it's configurable).
That said, if we add this feature, we should add a property to enable or disable it.
We can use [0, or any non-positive value, which is what sets whether or not the caching catalog has a TTL](https://github.com/apache/iceberg/blob/ae19482cd7f9ee3e9e95d7198cfc1e5068254d75/spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java#L401-L409).
Though catalogs general have a boolean property to enable / disable whether or not caching is used entirely. So either `iceberg.scan.enable-blocking-queue` or `iceberg.scan.use-shared-queue`, or just setting the value to 0 or -1 to disable it entirely
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1108116785
@rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1108124296
@rdblue `ParallelIterator ` use `ConcurrentLinkedQueue`, and it is a un-bouned queue. Sorry, could you please make me clear why the parallel iterator is self-limiting? FYI, the average file size in the table is ~ 1MB, and there are hundreds of thousands of datafiles in my scan. Following is a JVM heap snapshot:
<img width="819" alt="image" src="https://user-images.githubusercontent.com/7402327/165031548-de9fc13f-764b-449d-af1c-585a8b4e296d.png">
<img width="731" alt="image" src="https://user-images.githubusercontent.com/7402327/165031565-978ab449-8904-451e-b7cf-1c472a35dc18.png">
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1107929025
@uncleGen, what is the average file size in the table, the average number of files per manifest, and how many files is your scan producing? Is there anything consuming from the concurrent queue? This seems odd to me since the parallel iterator is self-limiting. When nothing is consuming from the queue, new tasks won't be submitted.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1136629488
@szehon-ho, that sounds correct to me. I'm reluctant to block the queue because that introduces another bottleneck in cases where you probably don't want one.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r882301640
##########
core/src/main/java/org/apache/iceberg/util/ParallelIterable.java:
##########
@@ -65,10 +67,10 @@ private ParallelIterator(Iterable<? extends Iterable<T>> iterables,
try (Closeable ignored = (iterable instanceof Closeable) ?
(Closeable) iterable : () -> { }) {
for (T item : iterable) {
- queue.add(item);
+ queue.put(item);
}
- } catch (IOException e) {
- throw new RuntimeIOException(e, "Failed to close iterable");
+ } catch (IOException | InterruptedException e) {
+ throw new RuntimeException("Failed to close iterable", e);
Review Comment:
BTW, I think we shouldn't call `Thread.sleep` in `hasNext` if we use blocking queue. In our test it's bad for performance when the queue size is small.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r855667791
##########
core/src/main/java/org/apache/iceberg/util/ParallelIterable.java:
##########
@@ -65,10 +67,10 @@ private ParallelIterator(Iterable<? extends Iterable<T>> iterables,
try (Closeable ignored = (iterable instanceof Closeable) ?
(Closeable) iterable : () -> { }) {
for (T item : iterable) {
- queue.add(item);
+ queue.put(item);
}
- } catch (IOException e) {
- throw new RuntimeIOException(e, "Failed to close iterable");
+ } catch (IOException | InterruptedException e) {
+ throw new RuntimeException("Failed to close iterable", e);
Review Comment:
Can we add a test case that goes through this path? I'm not sure if throwing when an `InterrruptedException` occurs is necessarily the correct behavior.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1125713285
Is there a reason that the files have to be 1MB? That’s very very small. You should also consider using Avro for storage at that size. But I think if you compacted files you probably wouldn’t have such issues during scan planning.
For ingest, sometimes we can’t avoid that small of files, I know. But scanning that many small files is counterintuitive for most cases. Ideally files that small (possibly coming from a 3rd party or a very very sharded system) would then be ingested into a table that is more ideally tailored to you query needs.
Do you run table maintenance actions on your table @uncleGen?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1127185882
> Is there a reason that the files have to be 1MB?
There is a streaming ingest job, it will create much small files.
> But I think if you compacted files you probably wouldn’t have such issues during scan planning.
> Do you run table maintenance actions on your table?
I have run `rewrite-files` action once a day. Ideally, reducing the interval may bypass this issue. But bypassing this issue should carefully consider jvm heap configuration with existing small files.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
openinx commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1144693457
Actually, I'd prefer to give my +1 to the bound queue solution. Because:
1. If there is an existing table which just has included too many manifests (and some of them just have many manifest entries), then the approaches will just don't work ( such as merging metadata so that those manifest size is an idea size, tuning the thread size etc). We can do nothing in the real production environment unless we increment the heap size of spark driver or trino coordinator. But what if we are not allowed to restart the spark driver & trino coordinator because of the other serving querying jobs ?
2. Does the blocking queue approach introduce any substantial performance bottleneck ? If we think the default blocking queue size is a bit small, then we can increase this default blocking queue size to 10000. I think most of the cases won't be effected by the default blocking queue size, unless we have an extremely large table with so many manifest entries. But in the case it seems to be easily OOM if we don't have any limited queue size.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r904514624
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
+
static boolean getBoolean(String systemProperty, boolean defaultValue) {
String value = System.getProperty(systemProperty);
if (value != null) {
return Boolean.parseBoolean(value);
}
return defaultValue;
}
+
+ public static int getInt(String systemProperty, int defaultValue) {
+ Preconditions.checkNotNull(systemProperty, "System property name should not be null");
Review Comment:
General but / FYI - we almost always tend to prefer plain English language with a structure similar to “general type of problem / problem phrase [possible solution only if not clear from stack trace]: offending value”.
we put the bad value at the end after a colon to give us a more continuous search string for logs (instead of mixing them into the sentence).
So for this situation, the suggested sentence would be format(“Invalid value for system property %s: null”, systemProperty);
However for this we can be more specific as null here means missing / not set almost always (and anybody who set it to null will see that). So I’d suggest format(“Invalid value for system property %s: null”, systemProperty); The final `: null` is debatable but I’d personally put it just to cover any cases where it might be explicitly null and just to match many others preconditions.
Yours is pretty good, but this tends to be our standard. I think it’s most helpful thinking of it in the context of the NPE or IllegalArumentException and full stack trace. What’s invariably helpful is having a common-ish way of writing these with a long enough, specific enough stack trace to be searchable, with less and less specific search query as the phrase is shortened.
Hope that helps for the long term! It really is just a nit but if you’re working on Iceberg offen enough it might help you as a contributor and a user.
```suggestion
Preconditions.checkNotNull(systemProperty, String.format("Invalid value for system property %s: null", systemProperty));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r904505957
##########
core/src/main/java/org/apache/iceberg/util/ParallelIterable.java:
##########
@@ -65,10 +67,10 @@ private ParallelIterator(Iterable<? extends Iterable<T>> iterables,
try (Closeable ignored = (iterable instanceof Closeable) ?
(Closeable) iterable : () -> { }) {
for (T item : iterable) {
- queue.add(item);
+ queue.put(item);
}
- } catch (IOException e) {
- throw new RuntimeIOException(e, "Failed to close iterable");
+ } catch (IOException | InterruptedException e) {
+ throw new RuntimeException("Failed to close iterable", e);
Review Comment:
+1 to not using Thread.sleep() anywhere if at all possible. It’s bad for system performance and generally causes instability at any scale. @lirui-apache’s result confirm what I’ve expected / known.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] zhangminglei commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
zhangminglei commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1137184238
Is there anything worse than OOM in production env if we do not use a blocking queue ? We may be able to use another solution to limit the flow rather than blocking queue that may cause another bottleneck.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1137703889
Good idea setting the number of threads. It could easily be that you're processing too many manifests in parallel because of the number of threads.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] lirui-apache commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
lirui-apache commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1136984014
Hey @szehon-ho , your understanding about the issue is correct. We did some test of iterating all manifest entries and compute aggregated stats for each partition. We tried various queue sizes ranging from 5 to 10000. In our test the consumer is pretty fast, and even the smallest queue doesn't affect the e2e latency of the job. The result might be different in other use cases where the consumer is not fast enough, but my hunch is that such job latency is bounded by consumer anyway.
One problem I can think of is when we plan files for multiple tables concurrently. And if one of the consumers is slow, it might block all the threads in the thread pool and prevent other jobs from making progress. We're investigating how to limit the resource used by each job.
In production env, we do have a background service to rewrite manifest periodically. But such optimization is asynchronous, which means if users query the table before the rewrite is done, it can still cause OOM.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#issuecomment-1106178031
Let me be clear. In my scene, my machine has 96 cores, when `iceberg.scan.plan-in-worker-pool` is enabled, the `iceberg.worker.num-threads` will be number of available processors, i.e. 96. So the input speed of queue is far greater than outpu speed of queue. Then OOM occurs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] uncleGen commented on a diff in pull request #4596: Use bounded queue to avoid consuming too much memory
Posted by GitBox <gi...@apache.org>.
uncleGen commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r857293968
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED = "iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE = "iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
Review Comment:
When use parallel iterator, `BaseFileScanTask` will be put into queue in parallel, while we consume these `BaseFileScanTask` serially. So it should not cause a performance bottleneck. We can make this size larger, like 10000, with more memory consuming.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org