Copilot commented on code in PR #7866:
URL: https://github.com/apache/ignite-3/pull/7866#discussion_r2983044344
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/SqlOutdatedPlanTest.java:
##########
@@ -217,8 +217,10 @@ private static class PrepareServiceSpy {
Semaphore semaphore = prepareBlockHolder.get();
try {
- semaphore.tryAcquire(10, TimeUnit.SECONDS);
- } catch (InterruptedException e) {
+ boolean acquired = semaphore.tryAcquire(10,
TimeUnit.SECONDS);
+
+ assertThat(acquired, is(true));
+ } catch (Throwable e) {
Review Comment:
Catching Throwable here will also catch AssertionError (and any Error), then
wrap it into RuntimeException, which makes test failures harder to diagnose and
can mask serious JVM errors. Prefer letting assertion failures propagate as-is,
and only handle InterruptedException explicitly (restoring the interrupt flag)
if needed.
```suggestion
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -1731,7 +1731,9 @@ public CompletableFuture<QueryPlan>
prepareAsync(ParsedResult parsedResult, SqlO
Runnable callback = prepareCallback;
if (callback != null) {
- callback.run();
+ // Run the callback asynchronously to avoid blocking the
calling thread.
+ return CompletableFuture.runAsync(callback)
+ .thenCompose(ignore ->
delegate.prepareAsync(parsedResult, ctx));
Review Comment:
Using CompletableFuture.runAsync(callback) (common ForkJoinPool) means the
subsequent delegate.prepareAsync(...) runs on that same common-pool thread (via
thenCompose). This can hang or fail in tests: (1) the callback may block (as in
SqlOutdatedPlanTest) and, together with other supplyAsync/runAsync calls that
block, can deadlock when common-pool parallelism is 1; (2) Ignite thread
assertions can fail because common-pool threads are not ThreadAttributes.
Consider running the callback+delegate invocation on an Ignite test thread
(e.g., IgniteTestUtils.runAsync / bypassingThreadAssertionsAsync) or providing
a dedicated executor/thread factory instead of the common 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
