You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "abstractdog (via GitHub)" <gi...@apache.org> on 2025/02/28 17:04:50 UTC

[PR] Core: lazy init workerPool [iceberg]

abstractdog opened a new pull request, #12427:
URL: https://github.com/apache/iceberg/pull/12427

   (no comment)


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2782430835

   > ./gradlew spotlessApply
   
   applied in the last commit, thanks for catching 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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2691216964

   I have seen a similar issue here: #12427 12220


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026549485


##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -153,6 +153,13 @@ public ExpireSnapshots planWith(ExecutorService executorService) {
     return this;
   }
 
+  protected ExecutorService planExecutorService() {
+    if (planExecutorService == null) {
+      this.planExecutorService = ThreadPools.getWorkerPool();
+    }

Review Comment:
   nit: new line please.
   The coding guide says we need to add new line after the blocks: https://iceberg.apache.org/contribute/#block-spacing



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026705782


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();
+    RemoveSnapshots removeSnapshots = (RemoveSnapshots) removeSnapshots(table).planWith(executor);
+
+    table.newAppend().appendFile(FILE_A).commit();
+    table.newAppend().appendFile(FILE_A).commit();
+
+    long tAfterCommits = waitUntilAfter(table.currentSnapshot().timestampMillis());
+    assertThrows(

Review Comment:
   ack



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1975827553


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   btw, looks like I forgot to replace with workerPool(), let me do quickly



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026551647


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +197,10 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    if (workerPool == null) {
+      this.workerPool = ThreadPools.getWorkerPool();
+    }
+    return workerPool;

Review Comment:
   nit: new line please.
   The coding guide says we need to add new line after the blocks: https://iceberg.apache.org/contribute/#block-spacing



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2691177743

   > Hi Laci, Long time, no see... What is the actual issue where you have find this problematic?
   Hey, long time no see Peter!
   
   the actual use-case was [HIVE-28759](https://issues.apache.org/jira/browse/HIVE-28759), where I'm about to supply an executorService to SnapshotProducer to be able to submit some jobs during HS2 shutdown
   
   even though the API supports scanManifestsWith, I wasn't able to achive my goal, because the static final variable was initialized in <clinit> when the class is first loaded and used...so this is an edge-case, when:
   1. HS2 starts
   2. there are some records in the query history queue that need to be flushed (anything that applies 3) )
   3. no iceberg operations using ThreadPools were running
   4. HS2 shutdown
   
   so I got an exception like:
   ```
   Caused by: java.lang.IllegalStateException: Shutdown in progress
   	at java.lang.ApplicationShutdownHooks.add(ApplicationShutdownHooks.java:66) ~[?:1.8.0_292]
   	at java.lang.Runtime.addShutdownHook(Runtime.java:211) ~[?:1.8.0_292]
   	at org.apache.iceberg.relocated.com.google.common.util.concurrent.MoreExecutors$Application.addShutdownHook(MoreExecutors.java:289) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.relocated.com.google.common.util.concurrent.MoreExecutors$Application.addDelayedShutdownHook(MoreExecutors.java:266) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.relocated.com.google.common.util.concurrent.MoreExecutors$Application.getExitingExecutorService(MoreExecutors.java:241) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.relocated.com.google.common.util.concurrent.MoreExecutors$Application.getExitingExecutorService(MoreExecutors.java:246) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.relocated.com.google.common.util.concurrent.MoreExecutors.getExitingExecutorService(MoreExecutors.java:129) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.ThreadPools.newWorkerPool(ThreadPools.java:89) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.ThreadPools.newWorkerPool(ThreadPools.java:85) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.ThreadPools.<clinit>(ThreadPools.java:45) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.SnapshotProducer.<init>(SnapshotProducer.java:102) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.MergingSnapshotProducer.<init>(MergingSnapshotProducer.java:103) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.MergeAppend.<init>(MergeAppend.java:32) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.BaseTable.newAppend(BaseTable.java:180) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.mr.hive.HiveIcebergOutputCommitter.commitWrite(HiveIcebergOutputCommitter.java:555) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.mr.hive.HiveIcebergOutputCommitter.commitTable(HiveIcebergOutputCommitter.java:494) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.mr.hive.HiveIcebergOutputCommitter.lambda$commitJobs$4(HiveIcebergOutputCommitter.java:292) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.Tasks$Builder.runTaskWithRetry(Tasks.java:413) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.Tasks$Builder.runSingleThreaded(Tasks.java:219) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:203) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:196) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.mr.hive.HiveIcebergOutputCommitter.commitJobs(HiveIcebergOutputCommitter.java:286) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.iceberg.mr.hive.HiveIcebergStorageHandler.storageHandlerCommit(HiveIcebergStorageHandler.java:828) ~[hive-iceberg-handler-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.hadoop.hive.ql.queryhistory.repository.IcebergRepository.flush(IcebergRepository.java:134) ~[hive-exec-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at org.apache.hadoop.hive.ql.queryhistory.QueryHistoryService.lambda$doFlush$1(QueryHistoryService.java:205) ~[hive-exec-3.1.3000.2025.0.20.0-SNAPSHOT.jar:3.1.3000.2025.0.20.0-SNAPSHOT]
   	at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1640) ~[?:1.8.0_292]
   	... 4 more
   ```
   
   even this is just an edge-case, the lazy init might look better in general


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026576164


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   This test doesn't fail without the change.
   Maybe you have meant `ThreadPools.getWorkerPool().shutdown();` - with that the test do not pass because `SnapshotProducer.writeManifests` has a hardcoded threadpool usage



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1989474677


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   `orElseGet` is lazy



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2782403538

   @abstractdog: Please apply spotless (`./gradlew spotlessApply`)


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026554669


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();
+    RemoveSnapshots removeSnapshots = (RemoveSnapshots) removeSnapshots(table).planWith(executor);
+
+    table.newAppend().appendFile(FILE_A).commit();
+    table.newAppend().appendFile(FILE_A).commit();
+
+    long tAfterCommits = waitUntilAfter(table.currentSnapshot().timestampMillis());
+    assertThrows(

Review Comment:
   Please use junit5: assertThatThrownBy
   We are in the process of migrating, and still both versions in the classpath, but we should use junit5



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2691159055

   Hi Laci,
   Long time, no see...
   What is the actual issue where you have find this problematic?


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1989472124


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   @abstractdog you should use `orElseGet` - it's lazy



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1989472124


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   @abstractdog you should use `orElseGet` - it's lazy



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


Re: [PR] Core: Lazy init workerPool in RemoveSnapshots and SnapshotProducer [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2782797362

   Merged to main.
   Thanks for the PR @abstractdog!


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


Re: [PR] Core: Lazy init workerPool in RemoveSnapshots and SnapshotProducer [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2783053740

   > Merged to main. Thanks for the PR @abstractdog!
   
   thanks a lot for the review comments @pvary!


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1989438499


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   If you don't mind, I would still prefer the first version where we call the getWorkerPool only once.



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026902452


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   Sorry for missing the comment! Went directly to the code, to check, and haven't seen your response 😭 
   
   What if we create a test very similar to `TestRewriteManifests.testRewriteManifestsWithScanExecutor` where we check that the threadpool is used and threads are created from it? It would have an advantage of failing is someone restores the static initialization.



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2028399067


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   makes sense, I just created: https://github.com/apache/iceberg/pull/12427/commits/144f265bad9b19281466958a0173b84114de1941
   
   I exactly copied testRewriteManifestsWithScanExecutor, which makes sense to validate that the passed executor is used, but still, the test case will pass if someone reverts the patch partially, like, only this:
   ```
    private ExecutorService planExecutorService = ThreadPools.getWorkerPool();
   ```



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026704750


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   how to mess with the central threadpool? please consider my comment: https://github.com/apache/iceberg/pull/12427#issuecomment-2763554614
   
   I mean, let's say I call:
   ```
   ThreadPools.getWorkerPool().shutdown()
   ```
   then what happens top everything other tests in the same JVM



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026883731


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   Thx!



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026704750


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   how to mess with the central threadpool? please consider my comment: https://github.com/apache/iceberg/pull/12427#issuecomment-2763554614
   
   I mean, let's say I call:
   ```
   ThreadPools.getWorkerPool().shutdown()
   ```
   then what happens to every other tests in the same JVM



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2030707295


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   okay, agreed, let's cover with test at least the fact that the custom executor is used



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2691235997

   > So let me recap:
   > 
   > * Hive have a task which needs to access some Iceberg tables during the shutdown
   > * During the shutdown the general threadpools are terminated
   > * Hive could provide their own threadpools to access the Iceberg tables
   > * And the issue is that the `ThreadPools.getWorkerPool()` is always called (and throws an exception) - even if it is not used
   
   exactly!


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2714565202

   Any idea how to add a unit test to prevent regression here?


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2763554614

   @pvary : added a testcase in TestRemoveSnapshots to check if the supplied executor service is used
   the same wasn't needed in case of SnapshotProducer as it already checked in another unit test, here: https://github.com/apache/iceberg/blob/d5971429ea903be873b5884c64a3dd41076179ea/core/src/test/java/org/apache/iceberg/TestRewriteManifests.java#L91
   
   recap: the main point of the patch was to prevent the early initialization of this.workerPool, and unit testing it is only possible with reflection, given the fact that it's private, not sure if I can contribute code like that
   
   pseudo-code
   ```
   SnapshotProducer snapshotProducer = new SnapshotProducer():
   assert(snapshotProducer.workerPool == null) // <-- reflection
   ```


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2030694266


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   Agreed, but I don't have a better idea ATM



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2691234726

   > I have seen a similar issue here: #12220
   
   thanks, it looks like the very same problem, so by using the default worker pool, we need to accept the default "exiting" behavior, which is fine, as long as users can use their own pools instead, that's what's 99% satisfied by scanManifestsWith interface in my case
   however, for that to work, iceberg should unblock it by lazily using the workerpool, otherwise the "Shutdown in progress" cannot be avoided


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1975825857


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   yeah, I think it's a cool one-liner, assuming that repeatedly calling ThreadPools::getWorkerPool is not expensive, which is true because it just returns the pool from the 2nd call:
   ```
     public static ExecutorService getWorkerPool() {
       return WORKER_POOL;
     }
   ``` 



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1975822162


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   I have checked the code, and there are some places where the `workerPool` is accessed directly.
   Is this a fancy way of telling:
   ```
   if (workerPool == null) {
       this.workerPool = ThreadPools.getWorkerPool();
   }
   
   return workerPool;
   ```



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#issuecomment-2691232543

   So let me recap:
   - Hive have a task which needs to access some Iceberg tables during the shutdown
   - During the shutdown the general threadpools are terminated
   - Hive could provide their own threadpools to access the Iceberg tables
   - And the issue is that the `ThreadPools.getWorkerPool()` is always called (and throws an exception) - even if it is not used


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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1975827553


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   btw, looks like I forgot to replace with workerPool(), let me do quickly
   FIXED in https://github.com/apache/iceberg/pull/12427/commits/e50b767d5885b994aa935c1d6ec70dc7f0d9fb5f



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r1989474677


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +198,7 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    return Optional.ofNullable(workerPool).orElseGet(ThreadPools::getWorkerPool);

Review Comment:
   `orElseGet` is lazy



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026549485


##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -153,6 +153,13 @@ public ExpireSnapshots planWith(ExecutorService executorService) {
     return this;
   }
 
+  protected ExecutorService planExecutorService() {
+    if (planExecutorService == null) {
+      this.planExecutorService = ThreadPools.getWorkerPool();
+    }

Review Comment:
   nit: new line please



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2026705407


##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -153,6 +153,13 @@ public ExpireSnapshots planWith(ExecutorService executorService) {
     return this;
   }
 
+  protected ExecutorService planExecutorService() {
+    if (planExecutorService == null) {
+      this.planExecutorService = ThreadPools.getWorkerPool();
+    }

Review Comment:
   ack



##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -197,7 +197,10 @@ protected String targetBranch() {
   }
 
   protected ExecutorService workerPool() {
-    return this.workerPool;
+    if (workerPool == null) {
+      this.workerPool = ThreadPools.getWorkerPool();
+    }
+    return workerPool;

Review Comment:
   ack
   



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


Re: [PR] Core: lazy init workerPool [iceberg]

Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #12427:
URL: https://github.com/apache/iceberg/pull/12427#discussion_r2028399067


##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1772,6 +1775,24 @@ public void testNoSchemasOrSpecsToRemove() {
                         .anyMatch(u -> u instanceof MetadataUpdate.RemoveSchemas)));
   }
 
+  @TestTemplate
+  public void testCustomWorkerPool() {
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    // shutdown the executor, cleanExpiredSnapshots should fail
+    executor.shutdownNow();

Review Comment:
   makes sense, I just created: https://github.com/apache/iceberg/pull/12427/commits/144f265bad9b19281466958a0173b84114de1941
   
   I exactly copied testRewriteManifestsWithScanExecutor, which makes sense to validate that the passed executor is used, but still, the test case will pass if someone reverts the patch partially, like, only this:
   ```
    private ExecutorService planExecutorService = ThreadPools.getWorkerPool();
   ```
   



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


Re: [PR] Core: Lazy init workerPool in RemoveSnapshots and SnapshotProducer [iceberg]

Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #12427:
URL: https://github.com/apache/iceberg/pull/12427


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