RFR: 8288783: JFR: Error messages are confusing when options conflict in -XX:StartFlightRecording
Could I have a review of PR that fixes incorrect error messages regarding of recording. The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output. It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean. For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording Testing: jdk/jdk/jfr Thanks Chihiro ------------- Commit messages: - modify whitespace from windows to linux - modify copyright - Modify log message and test case - JDK-8288783 Initial Changes: https://git.openjdk.org/jdk/pull/9302/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8288783 Stats: 442 lines in 10 files changed: 431 ins; 2 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
On Tue, 28 Jun 2022 05:50:38 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
This pull request has been closed without being integrated. ------------- PR: https://git.openjdk.org/jdk/pull/9302
Could you review this fix to make the error message more user friendly, please? Regards, Chihiro 2022年6月28日(火) 15:55 Chihiro Ito <cito@openjdk.org>:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
-------------
Commit messages: - modify whitespace from windows to linux - modify copyright - Modify log message and test case - JDK-8288783 Initial
Changes: https://git.openjdk.org/jdk/pull/9302/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8288783 Stats: 442 lines in 10 files changed: 431 ins; 2 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302
On Tue, 28 Jun 2022 05:50:38 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java line 77:
75: } 76: 77: protected void checkConflict() {
I think this part can be simplified, the variable names are long and there is too much state to track. How about this? `private void checkConflicted() { if (conflictedOptions.isEmpty()) { return; } StringBuilder sb = new StringBuilder(); if (conflictedOptions.size() == 1) { sb.append("Option "); sb.append(conflictedOptions.remove(0)); } else { sb.append("Options "); StringJoiner sj = new StringJoiner(", "); while (conflictedOptions.size() > 1) { sj.add(conflictedOptions.remove(0)); } sb.append(sj); sb.append(" and " + conflictedOptions.remove(0)); } sb.append(" can only be specified once."); throw new IllegalArgumentException(sb.toString()); }` It requires an ArrayList, but you could do: ` if (!conflictedOptions.contains(key)) { conflictedOptions.add(key); }` I don't think it's worth the complication with the JfrCommand enum to display the correct command being used in an error message. ------------- PR: https://git.openjdk.org/jdk/pull/9302
On Fri, 7 Oct 2022 05:26:44 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java line 77:
75: } 76: 77: protected void checkConflict() {
I think this part can be simplified, the variable names are long and there are too many branches/states to track.
How about this?
private void checkConflicted() { if (conflictedOptions.isEmpty()) { return; }
StringBuilder sb = new StringBuilder(); sb.append("Option"); if (conflictedOptions.size() > 1) { sb.append("s "); StringJoiner sj = new StringJoiner(", "); while (conflictedOptions.size() > 1) { sj.add(conflictedOptions.remove(0)); } sb.append(sj); sb.append(" and"); } sb.append(" "); sb.append(conflictedOptions.remove(0)); sb.append(" can only be specified once."); throw new IllegalArgumentException(sb.toString()); }
It requires an ArrayList, but you could do:
if (!conflictedOptions.contains(key)) { conflictedOptions.add(key); }
I don't think it's worth the complication with the JfrCommand enum to display the correct command being used in an unusual error message.
Thank you for your prompt reviewing. Your idea is good for me. I will change the code in that regard as soon as I can. I agree with the complication you mentiond. We posted about this in JBS with phrases like "with -XX:StartFlightRecording" at the end of the log message, and also mentioned jcmd. Howeber, at the existing implementation in logging the message, there is no way to identify a command such as "JFR.start". So I implemented it this way to identify the command as a last resort. As you say, I also think it no need to have a log message that identifies each command with the complication. Therefore, I will eliminate this complication. ------------- PR: https://git.openjdk.org/jdk/pull/9302
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision: eliminating complication such as outputting commands, and changing how to build log messages ------------- Changes: - all: https://git.openjdk.org/jdk/pull/9302/files - new: https://git.openjdk.org/jdk/pull/9302/files/1489318a..52316757 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=00-01 Stats: 158 lines in 10 files changed: 3 ins; 105 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision: reverting file headers ------------- Changes: - all: https://git.openjdk.org/jdk/pull/9302/files - new: https://git.openjdk.org/jdk/pull/9302/files/52316757..58237189 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=01-02 Stats: 12 lines in 6 files changed: 6 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
On Thu, 13 Oct 2022 06:07:49 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision:
reverting file headers
@egahlin Could you review this again, please? ------------- PR: https://git.openjdk.org/jdk/pull/9302
On Thu, 13 Oct 2022 06:07:49 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision:
reverting file headers
src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java line 126:
124: } else { 125: if (options.containsKey(key)) { 126: if(!conflictedOptions.contains(key)) {
Suggestion: if (!conflictedOptions.contains(key)) { test/jdk/jdk/jfr/jcmd/TestJcmdOptionSpecifiedOnce.java line 123:
121: try { 122: Matcher matcher = Pattern.compile(regex).matcher(output); 123: if(matcher.find()){
Suggestion: if (matcher.find()){ test/jdk/jdk/jfr/startupargs/TestStartupOptionSpecifiedOnce.java line 101:
99: try { 100: Matcher matcher = Pattern.compile(regex).matcher(output); 101: if(matcher.find()){
Suggestion: if (matcher.find()){ ------------- PR: https://git.openjdk.org/jdk/pull/9302
On Thu, 13 Oct 2022 06:07:49 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision:
reverting file headers
What worries me a bit is the testing. It is thorough, but perhaps too thorough? It might fail due to other reasons than the added logic, or it may become a maintenance burden. For example, the flush-interval option is planned to be removed. Starting 40 JVMs to verify an improved error message seems excessive. I would put the tests in one file, TestConflictingOptions and use options that are harmless (disk and name), but include "settings" as it should be repeatable, i.e void testJCmdConflict() { var output= JcmdHelper.jcmd("JFR.start name=hello name=greetings"); output.shouldContain("name can only be specified once") } void testStartFlightRecordingConflict() { var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:disk=true,disk=false,name=cat,name=dog"); output.shouldContain("disk and name can only be specified once.") } void testSettings() { var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:settings=default,settings=profile"); output.shouldNotContain("setting can only be specified once") } This should give 100% line coverage and make sure it works from both command line and jcmd. ------------- PR: https://git.openjdk.org/jdk/pull/9302
On Sat, 15 Oct 2022 09:03:38 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision:
reverting file headers
What worries me a bit is the testing. It is thorough, but perhaps too thorough?
It might fail due to other reasons than the added logic, or it may become a maintenance burden. For example, the flush-interval option is planned to be removed. Starting 40 JVMs to verify an improved error message seems excessive.
I would put the tests in one file, TestConflictingOptions and use options that are harmless (disk and name), but include "settings" as it should be repeatable, i.e
void testJCmdConflict() { var output= JcmdHelper.jcmd("JFR.start name=hello name=greetings"); output.shouldContain("name can only be specified once") }
void testStartFlightRecordingConflict() { var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:disk=true,disk=false,name=cat,name=dog"); output.shouldContain("disk and name can only be specified once.") }
void testSettings() { var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:settings=default,settings=profile"); output.shouldNotContain("settings can only be specified once") }
This should give 100% line coverage and make sure it works from both command line and jcmd.
@egahlin Thanks for the review. ------------- PR: https://git.openjdk.org/jdk/pull/9302
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/jdk/jfr/jcmd/TestJcmdOptionSpecifiedOnce.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> ------------- Changes: - all: https://git.openjdk.org/jdk/pull/9302/files - new: https://git.openjdk.org/jdk/pull/9302/files/58237189..3b901828 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with two additional commits since the last revision: - Update test/jdk/jdk/jfr/startupargs/TestStartupOptionSpecifiedOnce.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - Update src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> ------------- Changes: - all: https://git.openjdk.org/jdk/pull/9302/files - new: https://git.openjdk.org/jdk/pull/9302/files/3b901828..d1f68a7e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision: to simplify tests ------------- Changes: - all: https://git.openjdk.org/jdk/pull/9302/files - new: https://git.openjdk.org/jdk/pull/9302/files/d1f68a7e..bf395046 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=04-05 Stats: 218 lines in 2 files changed: 0 ins; 199 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision: - Merge branch 'openjdk:master' into 8288783 - to simplify tests - Update test/jdk/jdk/jfr/startupargs/TestStartupOptionSpecifiedOnce.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - Update src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - Update test/jdk/jdk/jfr/jcmd/TestJcmdOptionSpecifiedOnce.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - reverting file headers - eliminating complication such as outputting commands, and changing how to build log messages - modify whitespace from windows to linux - modify copyright - Modify log message and test case - ... and 1 more: https://git.openjdk.org/jdk/compare/d70aaed6...d276c4db ------------- Changes: - all: https://git.openjdk.org/jdk/pull/9302/files - new: https://git.openjdk.org/jdk/pull/9302/files/bf395046..d276c4db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9302&range=05-06 Stats: 771391 lines in 11248 files changed: 389735 ins; 257480 del; 124176 mod Patch: https://git.openjdk.org/jdk/pull/9302.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9302/head:pull/9302 PR: https://git.openjdk.org/jdk/pull/9302
On Wed, 8 Feb 2023 13:18:27 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
Chihiro Ito has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision:
- Merge branch 'openjdk:master' into 8288783 - to simplify tests - Update test/jdk/jdk/jfr/startupargs/TestStartupOptionSpecifiedOnce.java
Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - Update src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java
Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - Update test/jdk/jdk/jfr/jcmd/TestJcmdOptionSpecifiedOnce.java
Co-authored-by: Andrey Turbanov <turbanoff@gmail.com> - reverting file headers - eliminating complication such as outputting commands, and changing how to build log messages - modify whitespace from windows to linux - modify copyright - Modify log message and test case - ... and 1 more: https://git.openjdk.org/jdk/compare/45ce5690...d276c4db
Marked as reviewed by egahlin (Reviewer). ------------- PR: https://git.openjdk.org/jdk/pull/9302
On Tue, 28 Jun 2022 05:50:38 GMT, Chihiro Ito <cito@openjdk.org> wrote:
Could I have a review of PR that fixes incorrect error messages regarding of recording.
The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def, the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated. Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
It should say follows, Option name can only be specified once with starting flight recording Options name and disk can only be specified once with starting flight recording Options name, disk and maxage can only be specified once with starting flight recording This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
For other than start, the output is as follows, (example of dump) Option name can only be specified once with dumping flight recording
Testing: jdk/jdk/jfr
Thanks Chihiro
This pull request has now been integrated. Changeset: 36478ee1 Author: Chihiro Ito <cito@openjdk.org> URL: https://git.openjdk.org/jdk/commit/36478ee13f0877447852470150c01397388b3f82 Stats: 138 lines in 3 files changed: 135 ins; 1 del; 2 mod 8288783: Error messages are confusing when options conflict in -XX:StartFlightRecording Reviewed-by: egahlin ------------- PR: https://git.openjdk.org/jdk/pull/9302
participants (5)
-
Andrey Turbanov
-
Chihiro Ito
-
Chihiro Ito
-
duke
-
Erik Gahlin