Update airbase to 140 #17740

wendigo · 2023-06-02T19:58:03Z

This change makes it possible to build project with project.build.targetJdk 21.

The only remaining maven infrastructure bit that doesn't work under JDK 21 is error-prone but the current HEAD fixes those problems so it's a matter of updating error-prone when the next version is released.

This also brings Guava to version 32.0.0 (https://github.com/google/guava/releases/tag/v32.0.0)

client/trino-cli/src/main/java/io/trino/cli/Console.java

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/split/HudiBackgroundSplitLoader.java

wendigo · 2023-06-02T20:08:00Z

@martint these are the only violations after Guava dropped @CanIgnoreReturnValue from a bunch of methods. There are no new violations with updated Guava and error-prone HEAD version besides those in that PR.

losipiuk · 2023-06-05T09:13:37Z

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/split/HudiBackgroundSplitLoader.java

@@ -62,6 +62,7 @@ public HudiBackgroundSplitLoader(
        this.hudiSplitFactory = new HudiSplitFactory(tableHandle, hudiSplitWeightProvider);
    }

+    @SuppressWarnings("CheckReturnValue")


why exactly? (the links from commit desc do not explain)

According to the FutureCombiner javadoc and error-prone FutureReturnValueIgnored:

Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future. The problem Methods that return java.util.concurrent.Future and its subclasses generally indicate errors by returning a future that eventually fails. If you don’t check the return value of these methods, you will never find out if they threw an exception. Nested futures can also result in missed cancellation signals or suppressed exceptions - see [Avoiding Nested Futures](https://github.com/google/guava/wiki/ListenableFutureExplained#avoid-nested-futures) for details.

Document why it's correct to do this suppression. Ask Hudi people if we don't know

I've hooked the same listener on this combined future as in other places.

@codope @7c00 does it look right to you?

hashhar

LGTM % Lukasz's comment. suppressions should have reasons ideally so that people can revisit them

client/trino-cli/src/main/java/io/trino/cli/Console.java

...tor-legacy/src/test/java/io/trino/plugin/raptor/legacy/util/TestPrioritizedFifoExecutor.java

client/trino-cli/src/main/java/io/trino/cli/Console.java

This change makes it possible to build project with project.build.targetJdk 21. Additionally suppress error-prone `CheckReturnValue` violations with Guava 32.0.0 These are the result of following changes: - google/guava@a7f6b08 - google/guava@0ef6688

findepi · 2023-06-05T12:40:40Z

...tor-legacy/src/test/java/io/trino/plugin/raptor/legacy/util/TestPrioritizedFifoExecutor.java

@@ -70,7 +70,8 @@ public void testCounter()
            futures.add(executor.submit(() -> {
                try {
                    // wait for the go signal
-                    awaitUninterruptibly(startLatch, 1, TimeUnit.MINUTES);
+                    @SuppressWarnings("CheckReturnValue")
+                    boolean ignored = awaitUninterruptibly(startLatch, 1, TimeUnit.MINUTES);


why ignore?

To leave the behavior as it was before.

this is a test, so it would not be unsafe to just improve it

Agreed, I would make all these assertTrue(...)

codope · 2023-06-06T03:58:50Z

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/split/HudiBackgroundSplitLoader.java

@@ -74,7 +74,7 @@ public void start()
                            .map(partition -> Futures.submit(() -> loadSplits(partition), executor))
                            .peek(this::hookErrorListener)
                            .collect(Collectors.toList());
-                    Futures.whenAllComplete(futures).run(asyncQueue::finish, directExecutor());
+                    hookErrorListener(Futures.whenAllComplete(futures).run(asyncQueue::finish, directExecutor()));


@wendigo Shouldn't the hook on line 81 be sufficient?

It’s about ignoring return value of the combiner call. What would you propose instead adding a hook?

@codope here we add a hook for asyncQueue::finish potential failures

Got it, thanks

I’ve migrated this change to other PR. Seems to work just fine

wendigo · 2023-06-07T04:16:00Z

Superseded by #17770

cla-bot bot added the cla-signed label Jun 2, 2023

wendigo requested review from electrum, kokosing, findepi and martint June 2, 2023 19:58

wendigo mentioned this pull request Jun 2, 2023

[JDK 21] Build and run Trino on JDK 21 #17017

Closed

13 tasks

martint reviewed Jun 2, 2023

View reviewed changes

client/trino-cli/src/main/java/io/trino/cli/Console.java Outdated Show resolved Hide resolved

wendigo commented Jun 2, 2023

View reviewed changes

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/split/HudiBackgroundSplitLoader.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/airbase-140 branch from b0e8fd8 to 7d9e25c Compare June 2, 2023 20:24

wendigo requested a review from martint June 2, 2023 20:26

wendigo force-pushed the serafin/airbase-140 branch from 7d9e25c to 71f6086 Compare June 2, 2023 20:29

github-actions bot added the hudi Hudi connector label Jun 2, 2023

wendigo requested a review from hashhar June 5, 2023 08:52

losipiuk reviewed Jun 5, 2023

View reviewed changes

losipiuk approved these changes Jun 5, 2023

View reviewed changes

hashhar reviewed Jun 5, 2023

View reviewed changes

findepi reviewed Jun 5, 2023

View reviewed changes

client/trino-cli/src/main/java/io/trino/cli/Console.java Outdated Show resolved Hide resolved

findepi reviewed Jun 5, 2023

View reviewed changes

...tor-legacy/src/test/java/io/trino/plugin/raptor/legacy/util/TestPrioritizedFifoExecutor.java Outdated Show resolved Hide resolved

findepi reviewed Jun 5, 2023

View reviewed changes

client/trino-cli/src/main/java/io/trino/cli/Console.java Outdated Show resolved Hide resolved

Update airbase to 140

ddb7b09

This change makes it possible to build project with project.build.targetJdk 21. Additionally suppress error-prone `CheckReturnValue` violations with Guava 32.0.0 These are the result of following changes: - google/guava@a7f6b08 - google/guava@0ef6688

wendigo force-pushed the serafin/airbase-140 branch from 71f6086 to ddb7b09 Compare June 5, 2023 12:28

findepi reviewed Jun 5, 2023

View reviewed changes

codope reviewed Jun 6, 2023

View reviewed changes

wendigo mentioned this pull request Jun 7, 2023

Update to Jakarta servlet and JAX-RS dependencies #17770

Merged

wendigo closed this Jun 7, 2023

wendigo deleted the serafin/airbase-140 branch June 7, 2023 04:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update airbase to 140 #17740

Update airbase to 140 #17740

wendigo commented Jun 2, 2023 •

edited

Loading

wendigo commented Jun 2, 2023

losipiuk Jun 5, 2023

wendigo Jun 5, 2023

findepi Jun 5, 2023

wendigo Jun 5, 2023

wendigo Jun 5, 2023

hashhar left a comment

findepi Jun 5, 2023

wendigo Jun 5, 2023

findepi Jun 5, 2023

electrum Jun 5, 2023

codope Jun 6, 2023

wendigo Jun 6, 2023

findepi Jun 6, 2023

codope Jun 7, 2023

wendigo Jun 7, 2023

wendigo commented Jun 7, 2023

Update airbase to 140 #17740

Update airbase to 140 #17740

Conversation

wendigo commented Jun 2, 2023 • edited Loading

wendigo commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendigo commented Jun 7, 2023

wendigo commented Jun 2, 2023 •

edited

Loading