-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8350824: New async logging gtest StallingModePreventsDroppedMessages fails #23819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into |
@jdksjolen This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 16 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@jdksjolen The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
Hi @MBaesken , @dholmes-ora Would you mind reviewing this? Thank you! |
bool async = AsyncLogWriter::instance() != nullptr | ||
&& LogConfiguration::async_mode() == LogConfiguration::AsyncMode::Drop; | ||
if (async) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests do not currently fail, as the only mode gtests are run in are drop
mode. However, let's fix them now in order to avoid failures in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay.
Thanks
Thank you /integrate |
Going to push as commit ac76d8d.
Your commit was automatically rebased without conflicts. |
@jdksjolen Pushed as commit ac76d8d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
When I added async stalling mode I also added this gtest early in the process, and as tier1/GHA passed I did not look at the test again. However, the test does not run until much deeper in the CI/CD process for us, as that's when we turn on async logging explicitly for all gtests.
Looking at it now, it's clearly buggy. It doesn't check which async mode UL is in, and as
drop
is default, that makes it bound to fail. Besides, the goal of it is to check whether async stalling mode drops messages or not during stress. We already have an excellent test for that in JTReg, and this does run early as it spawns its own VM with the correct asynchronous mode specified. My suggested fix is to delete the gtest and add a dropped message check to the JTReg test instead.Testing: StressAsyncUL.java, GHA
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23819/head:pull/23819
$ git checkout pull/23819
Update a local copy of the PR:
$ git checkout pull/23819
$ git pull https://git.openjdk.org/jdk.git pull/23819/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23819
View PR using the GUI difftool:
$ git pr show -t 23819
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23819.diff
Using Webrev
Link to Webrev Comment