RFR: 8288783: JFR: Error messages are confusing when options conflict in -XX:StartFlightRecording

[Erik Gahlin]

On Tue, 28 Jun 2022 05:50:38 GMT, Chihiro Ito <cito at 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