RFR: 8335907: JFR: Make SettingControls more robust [v3]

Markus Grönlund mgronlun at openjdk.org
Tue Jul 30 12:29:33 UTC 2024


On Mon, 29 Jul 2024 11:12:12 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> Could I have a review of changes to the SettingControl implementations to make them more robust:
>> 
>> - Consistently ignore invalid values for the methods `combine(Set)` and `setValue(String)`.
>> - Consistently set the textual value field in the `SettingControl` after the actual value has been changed.
>> - Consistently set the textual value field to the default value in the constructor.
>> 
>> A test was added to ensure that current and future setting controls are properly tested. This also improves test coverage. See [JDK-8309052](https://bugs.openjdk.org/browse/JDK-8309052)
>> 
>> **Implementation comments:**
>> 
>> - A `jdk.jfr.internal.util.Rate` class was added to make the code more readable.
>> - The `ValueParser$TimespanUnit` class was made a top-level class for unit handling.
>> - A `Utils::multiplyOverflow(long, long, long)` method was added to avoid negative values in case of an overflow.
>> - `String::strip` was used instead of `String::trim` to make the implementation Unicode aware.
>> - The cutoff setting in the `EventControl` class was changed to “infinity”, which in practice was already the default value.
>> 
>> Composition, which is typically preferable over inheritance, was previously used for the `EnabledSetting` and `StackTraceSetting` classes. However, the code became hard to read and error-prone, so there is now an abstract `BooleanSetting` class to enforce consistency.
>> 
>> **Testing:**
>>  - test/jdk/jdk/jfr/*
>>  - 100 * test/jdk/jdk/jfr/api/setting/TestSettingControl.java 
>> 
>> **Out of scope:**
>> - Formatting issues
>> - Wider use of the TimespanUnit enum
>
> Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Move use of level to after Objects.requireNonNull

Marked as reviewed by mgronlun (Reviewer).

-------------

PR Review: https://git.openjdk.org/jdk/pull/20336#pullrequestreview-2207546331


More information about the hotspot-jfr-dev mailing list