RFR: 8340113: Remove JULONG as a Diagnostic Command argument type (jcmd JFR.view)
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT. ------------- Commit messages: - Merge remote-tracking branch 'upstream/master' into 8340113_JULONG - 8340113: Remove JULONG as a Diagnostic Command argument type (jcmd JFR.view) Changes: https://git.openjdk.org/jdk/pull/21021/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8340113 Stats: 13 lines in 4 files changed: 0 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/21021.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21021/head:pull/21021 PR: https://git.openjdk.org/jdk/pull/21021
On Mon, 16 Sep 2024 15:39:58 GMT, Kevin Walls <kevinw@openjdk.org> wrote:
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Use of the operation parameter type description "JULONG" in Diagnostic Commands should be changed to INT. DiagnosticCommandMBean provides JMX/MBean access to DiagnosticCommands, aka jcmds, as JMX Operations (https://docs.oracle.com/en/java/javase/22/docs/api/jdk.management/com/sun/ma...). Parameter types for the operations are available. The list of possible types for parameters is not specified, it's left as "implementation dependent" (see DiagnosticCommandArgumentInfo). Applications such as JMC have historical prior knowledge of the "type" strings to expect. But even JMC does not understand "JULONG". It should be replaced with "INT", which most diagnostic commands use for all integer values, mostly (all?) unsigned. The JULONG type name has also appeared in jfrOptionSet.cpp for parsing the native FlightRecorderOptions. This does not get exposed to the MBean, but using JULONG/JINT as a type name still looks out of place. It can be changed with no impact. Tests keep passing: nothing expects or relies on the presence of "JULONG", e.g. TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/com/sun/management/DiagnosticCommandMBean 4 4 0 0 jtreg:test/jdk/jdk/jfr 598 598 0 0 ------------- PR Comment: https://git.openjdk.org/jdk/pull/21021#issuecomment-2353312884
On Mon, 16 Sep 2024 15:39:58 GMT, Kevin Walls <kevinw@openjdk.org> wrote:
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Hi Kevin, I had some questions about this change. Regarding the `INTEGER` -> `INT` change, I was wondering if the plan was to also update that for the remaining JFR dcmds ([DcmdView.java](https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr...), [DcmdDump.java](https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr...), etc) and whether those changes should go in together? Also, I noticed the `getOptions` text is wrong for `width` in [DcmdQuery.java](https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr...). width (Optional) Maximum number of horizontal characters. (BOOLEAN, false) Perhaps you can also fix this with this PR. Lastly, for `ArgumentParser.java`, I was thinking you could change `parseLong` to `parseInt`. However, I noticed that in [QueryRecording.java](https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr...), I see `width` and `cell-height` are downgraded to ints, whereas `maxAge` is used as a Long. So perhaps it should stay that way and it's something to note for anyone else looking at the PR :-) Thanks! ------------- PR Comment: https://git.openjdk.org/jdk/pull/21021#issuecomment-2353910457
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Additional typos/default values ------------- Changes: - all: https://git.openjdk.org/jdk/pull/21021/files - new: https://git.openjdk.org/jdk/pull/21021/files/16b25717..393b5690 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/21021.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21021/head:pull/21021 PR: https://git.openjdk.org/jdk/pull/21021
On Tue, 17 Sep 2024 08:44:41 GMT, Kevin Walls <kevinw@openjdk.org> wrote:
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
Additional typos/default values
Thanks Sonia - Happy to tweak other help items as part of this. DCmdQuery is not enabled in any build except through a source change, but yes that width is clearly an INT. DCmdView has the same width param, but states it has no default, when the default is actually 100. Fixing these, and one other typo in an example. Yes, ArgumentParser I thought best to leave alone for now, callers handle it how they need. The other INTEGERs yes can standardise those in: src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdDump.java src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdStart.java Thanks! ------------- PR Comment: https://git.openjdk.org/jdk/pull/21021#issuecomment-2354976215
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Addition INTEGER to standard INT ------------- Changes: - all: https://git.openjdk.org/jdk/pull/21021/files - new: https://git.openjdk.org/jdk/pull/21021/files/393b5690..d79874df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/21021.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21021/head:pull/21021 PR: https://git.openjdk.org/jdk/pull/21021
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: No comma in: INT followed by... ------------- Changes: - all: https://git.openjdk.org/jdk/pull/21021/files - new: https://git.openjdk.org/jdk/pull/21021/files/d79874df..d7706785 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21021&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/21021.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21021/head:pull/21021 PR: https://git.openjdk.org/jdk/pull/21021
On Tue, 17 Sep 2024 14:32:22 GMT, Kevin Walls <kevinw@openjdk.org> wrote:
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
No comma in: INT followed by...
Marked as reviewed by lmesnik (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/21021#pullrequestreview-2311339024
On Tue, 17 Sep 2024 14:32:22 GMT, Kevin Walls <kevinw@openjdk.org> wrote:
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
No comma in: INT followed by...
Looks good. Have you run the jfr tests in test/jdk/jdk/jfr ------------- Marked as reviewed by egahlin (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21021#pullrequestreview-2312420090
On Wed, 18 Sep 2024 11:35:32 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Looks good. Have you run the jfr tests in test/jdk/jdk/jfr
Thanks Leonid and Erik for reviewing. Yes, all tests are passing. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21021#issuecomment-2358234791
On Mon, 16 Sep 2024 15:39:58 GMT, Kevin Walls <kevinw@openjdk.org> wrote:
The few uses of the operation parameter type "JULONG" in Diagnostic Commands should be changed to INT.
This pull request has now been integrated. Changeset: 19b2cee4 Author: Kevin Walls <kevinw@openjdk.org> URL: https://git.openjdk.org/jdk/commit/19b2cee42081e1f8e9c53e6c831ce1d2d2915fd5 Stats: 19 lines in 6 files changed: 0 ins; 0 del; 19 mod 8340113: Remove JULONG as a Diagnostic Command argument type (jcmd JFR.view) Reviewed-by: lmesnik, egahlin ------------- PR: https://git.openjdk.org/jdk/pull/21021
participants (4)
-
Erik Gahlin
-
Kevin Walls
-
Leonid Mesnik
-
Sonia Zaldana Calles