RFR: 8335907: JFR: Make SettingControls more robust
Could I have 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 - Clarify the SettingControl specification on how invalid values should be handled ------------- Commit messages: - Minor update - Initial Changes: https://git.openjdk.org/jdk/pull/20336/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335907 Stats: 911 lines in 17 files changed: 573 ins; 264 del; 74 mod Patch: https://git.openjdk.org/jdk/pull/20336.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20336/head:pull/20336 PR: https://git.openjdk.org/jdk/pull/20336
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 - Clarify the SettingControl specification on how invalid values should be handled
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Simplify implementation of BooleanSetting::combine ------------- Changes: - all: https://git.openjdk.org/jdk/pull/20336/files - new: https://git.openjdk.org/jdk/pull/20336/files/01488537..76203942 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=00-01 Stats: 13 lines in 1 file changed: 3 ins; 7 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20336.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20336/head:pull/20336 PR: https://git.openjdk.org/jdk/pull/20336
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 - Clarify the SettingControl specification on how invalid values should be handled
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Move use of level to after Objects.requireNonNull ------------- Changes: - all: https://git.openjdk.org/jdk/pull/20336/files - new: https://git.openjdk.org/jdk/pull/20336/files/76203942..fdeed890 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20336.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20336/head:pull/20336 PR: https://git.openjdk.org/jdk/pull/20336
On Mon, 29 Jul 2024 11:12:12 GMT, Erik Gahlin <egahlin@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
On Thu, 25 Jul 2024 19:59:11 GMT, Erik Gahlin <egahlin@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
This pull request has now been integrated. Changeset: 41486131 Author: Erik Gahlin <egahlin@openjdk.org> URL: https://git.openjdk.org/jdk/commit/41486131481164a559aa534807fe1a77a7d29fc8 Stats: 907 lines in 17 files changed: 569 ins; 264 del; 74 mod 8335907: JFR: Make SettingControls more robust Reviewed-by: mgronlun ------------- PR: https://git.openjdk.org/jdk/pull/20336
participants (2)
-
Erik Gahlin
-
Markus Grönlund