[improve][cli] pulsar-perf: check for invalid CLI options

[pgier]

Fixes #18866

Motivation

pulsar-perf incorrectly interprets bad options (e.g. --foo) as topic names. This results in an unclear error message if, for example, there is a typo in one of the options.

Modifications

I looked through the code for the JCommander library to see if there is a built-in argument validation for things like this, however, JCommander is kind of limited in this respect. If you accept any non-option arguments, then it's up to you to validate those, so I had to add arg/topic validation to the pulsar-perf code.

Check the non-option arguments for values starting with a '-'. If found, print an error message that there was an un-recognized option.

Old error message

> bin/pulsar-perf produce --foo mytopic
The size of topics list should be equal to --num-topic
Usage: pulsar-perf produce [options] persistent://prop/ns/my-topic
  Options:
    -am, --access-mode
      Producer access mode
  ...

New error message

> bin/pulsar-perf produce --foo mytopic
invalid option: '--foo'
To use a topic with the name '--foo', please use a fully qualified topic name
Usage: pulsar-perf produce [options] persistent://prop/ns/my-topic
  Options:
    -am, --access-mode
      Producer access mode
  ...

Verifying this change

• Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: pgier#6

[pgier]

There is currently some code repetition between pulsar-perf consume, produce, and read, however, removing this duplication requires a larger refactoring, and I think is outside of the scope of this change. I'll try to work on a separate PR which can share more code between these commands.

[tisonkun]

Interesting.

This should have been handled by JCommander. But it seems that the version of jc we depend on cannot regard these cases as unknown options.

I read the source code of JCommander#parseValues and it shows the case...

For this patch, we may factor out this three code snippet to a utility and name it as validating params?

[tisonkun]

removing this duplication requires a larger refactoring, and I think is outside of the scope of this change

OK. I read this sentence now.

Another question is that: is it possible a valid topic name starts with -?

[pgier]

@tisonkun AFAICT even the latest versions of JCommander don't handle this. If you specify an unnamed set of args, like we do for the topics, JCommander will interpret any unknown options as args, and there doesn't seem to be a way to check for this other than the manual check.

Yes, pulsar allows topics that start with -, although I would probably suggest that it shouldn't. I wasn't sure how common that use case is, that's why I put a note in the error message that a user can just use a fully qualified topic name if they have a topic with a -.

[tisonkun]

Thanks for your explanation! Good to go.

[tisonkun]

/pulsarbot run-failure-checks

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.94%. Comparing base (3180a4a) to head (2a38659).
Report is 1988 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18889      +/-   ##
============================================
+ Coverage     46.17%   46.94%   +0.77%     
- Complexity    10359    10534     +175     
============================================
  Files           703      703              
  Lines         68845    68861      +16     
  Branches       7382     7383       +1     
============================================
+ Hits          31788    32330     +542     
+ Misses        33448    32906     -542     
- Partials       3609     3625      +16     

Flag	Coverage Δ
unittests	46.94% <ø> (+0.77%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 73 files with indirect coverage changes

[eolivelli]

LGTM

thank you !

[lhotari]

Good work @pgier