Skip to content

Update airbase to version 124#11900

Closed
nineinchnick wants to merge 3 commits intotrinodb:masterfrom
nineinchnick:bump-airbase
Closed

Update airbase to version 124#11900
nineinchnick wants to merge 3 commits intotrinodb:masterfrom
nineinchnick:bump-airbase

Conversation

@nineinchnick
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick commented Apr 11, 2022 •
edited
Loading

Description

This version updated maven-surefire-plugin to 3.0.0-M6,
which should reduce maximum memory usage in tests.

Is this change a fix, improvement, new feature, refactoring, or other?

other

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

test execution

How would you describe this change to a non-technical end user or system administrator?

n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 11, 2022
@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented Apr 11, 2022

#11856 is required first.

@wendigo wendigo mentioned this pull request Apr 12, 2022
Closed
@findepi
Copy link
Copy Markdown
Member

findepi commented Apr 12, 2022

@nineinchnick build is red.

can you also resolve #11294 TODOs ?

Jan Was added 2 commits April 13, 2022 08:24
Update airbase to version 124
c6a0099 
This version updated maven-surefire-plugin to 3.0.0-M6,
which should reduce maximum memory usage in tests.
Restore default order of test execution
e16a7af
@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented Apr 13, 2022

trino-main failed with:

Terminating due to java.lang.OutOfMemoryError: Java heap space

This should be easy to reproduce locally. I'll investigate since the surefire update was supposed to help with max mem usage.

trino-mysql fails with multiple errors like:

 Error:  io.trino.plugin.mysql.TestMySqlTableStatisticsMySql8Histograms.testNotAnalyzed  Time elapsed: 0.997 s  <<< FAILURE!
java.lang.AssertionError: 

Expecting:
  <2733.0>
to be close to:
  <15000.0>

which is easily reproducible locally, but also locally on master. There might be something about how I run the tests, my env or the test might be flaky, but this doesn't show up in CI on master.

Anyway, it'll take me a while to sort these things out.

@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented Apr 13, 2022

I also just ran into this again locally in trino-main:

[INFO] Running TestSuite
2022-04-13T03:31:11.078-0500 INFO LogTestDurationListener enabled: true
[ERROR] 
Cannot instantiate class io.trino.execution.TestSqlTask

@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented Apr 13, 2022

We're hitting this issue https://issues.apache.org/jira/browse/SUREFIRE-2068 that's not present in M5, looks like this would be an upgrade blocker.

@findepi
Copy link
Copy Markdown
Member

findepi commented Apr 13, 2022

We're hitting this issue https://issues.apache.org/jira/browse/SUREFIRE-2068 that's not present in M5, looks like this would be an upgrade blocker.

It seems we should downgrade surefire in airbase. So sad.

@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented Apr 13, 2022

We're hitting this issue https://issues.apache.org/jira/browse/SUREFIRE-2068 that's not present in M5, looks like this would be an upgrade blocker.

It seems we should downgrade surefire in airbase. So sad.

I agree but I'll check what exactly triggers this and if there workarounds.

findepi
findepi reviewed Apr 13, 2022
View reviewed changes
core/trino-main/src/test/java/io/trino/execution/TestingPageSourceProvider.java
{
public TestingPageSourceProvider()
{
System.out.println();
Copy link
Copy Markdown
Member

@findepi findepi Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

anyway, crashing whenever we have System.out isn't practcal....

Copy link
Copy Markdown
Member Author

@nineinchnick nineinchnick Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we probably won't do this update, but I wanted to check if that'll work and are there going to be any gainst in job times.

I don't want to start bikeshedding, but are there any best practices about tests using logging or printing directly?

Copy link
Copy Markdown
Member

@findepi findepi Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i usually prefer logging to direct sout

Copy link
Copy Markdown
Member

@hashhar hashhar Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Surefire itself doesn't like sout since it uses that for passing control messages and crashes depending on when a sout is done.

Newer versions have this fixed though it seems but required some config - https://stackoverflow.com/a/61936537/4594699

Copy link
Copy Markdown
Member

@findepi findepi Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newer versions have this fixed though it seems but required some config - https://stackoverflow.com/a/61936537/4594699

for posterity

  • this references https://issues.apache.org/jira/browse/SUREFIRE-1614
  • and suggests to use 
    <plugin>
   <artifactId>maven-surefire-plugin</artifactId>
   <version>3.0.0-M5</version>
   <configuration>
     <!-- Activate the use of TCP to transmit events to the plugin -->
     <forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory"/>
   </configuration>
 </plugin>

Copy link
Copy Markdown
Member Author

@nineinchnick nineinchnick Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment in the Surefire Jira issue that it is supposed to be fixed on master, so I guess we can wait for the next release. I'll close this PR until then.

Copy link
Copy Markdown
Member

@findepi findepi May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow-up: downgrade surefire in airbase: airlift/airbase#313

@findepi findepi mentioned this pull request May 18, 2022
Merged
@martint martint mentioned this pull request Jun 1, 2022
Merged
@nineinchnick nineinchnick mentioned this pull request Jun 9, 2022
Closed
@nineinchnick nineinchnick mentioned this pull request Apr 19, 2023
Merged
@nineinchnick nineinchnick deleted the bump-airbase branch May 27, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi left review comments

@hashhar hashhar hashhar left review comments

Assignees

No one assigned

Labels

cla-signed

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

trino-hive tests sometimes fail with OutOfMemoryError Remove test concurrency related workarounds

3 participants

@nineinchnick @findepi @hashhar