-
Notifications
You must be signed in to change notification settings - Fork 174
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
JMC-6141: JMC logs warnings when creating recordings #270
Conversation
👋 Welcome back aptmac! A progress list of the required criteria for merging this PR into |
Webrevs
|
I don't see it in the PR, have you forget to add it? |
Ah yes I had forgotten to push it, it should be included now. |
@aptmac 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit e34323d. |
This PR addresses JMC-6141 [0], in which JMC logs warnings when creating recordings. This is particularly annoying when running unit tests because the terminal gets flooded with warnings.
There are two problems here.
The first problem is that the
JFR_SETTINGS_PERIOD
constant in EventTypeMetadataV2 is outdated. It is currently hardcoded ascom.oracle.jfr.settings.Period
which looks to have been the name prior to open-sourcing [1], but has since becomejdk.settings.Period
[2][3]. This removes the warnings related toWARNING: Inferred content type 'jdk.jfr.Period' for option Period
.The second problem is that there isn't any handling for the value "infinity" when parsing the timespan. The method
parsePersisted()
in LinearKindOfQuantity looks at the persisted string and uses regex to separate the unit from the value. In this case with period however, if the timespan is infinite then the string is just "infinity", so the regex doesn't work and we end up throwing an exception. I took some inspiration from the jfr code [4], and it handles infinite timespans by checking the string, and if it is "infinity" then returns the max long value [5]. I've added a unit test to reinforce this behaviour.Let me know if this approach is okay, and if there are different options I need to consider.
After these two changes my test output is legible, and I haven't noticed and regressions with uitests or my manual testing. When doing a regular
mvn clean verify
for the application we see a reduction of 12800 logged lines.[0] https://bugs.openjdk.java.net/browse/JMC-6141
[1] https://github.com/alibaba/dragonwell8_jdk/blob/423386885af50cf6f956d1b1ba159a994045a8ae/src/share/classes/jdk/jfr/internal/settings/PeriodSetting.java/#L43
[2] https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/PeriodSetting.java#L42
[3] https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr/internal/Type.java#L53
[4] https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/PeriodSetting.java#L116
[5] https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java#L304
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/270/head:pull/270
$ git checkout pull/270
Update a local copy of the PR:
$ git checkout pull/270
$ git pull https://git.openjdk.java.net/jmc pull/270/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 270
View PR using the GUI difftool:
$ git pr show -t 270
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/270.diff