Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][cli] pulsar-perf: refactor to reduce code duplication #19279

Merged
merged 1 commit into from
May 22, 2023

Conversation

pgier
Copy link
Contributor

@pgier pgier commented Jan 18, 2023

Motivation

Some of the code in the pulsar-perf command line tools seems to be unnecessarily repetitive.

Modifications

This removes some code duplication in the pulsar-perf utilities by pushing the common CLI parsing logic up into the shared PerformanceBaseArguments class. This also deprecates some inconsistent CLI arg naming, and make some improvements to the CLI arg validation.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as the pulsar-perf tests.

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
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: pgier#8

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@nicoloboschi PTAL

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @pgier !

@lhotari
Copy link
Member

lhotari commented Jan 31, 2023

closing and re-opening to run CI with latest changes from master.

@lhotari lhotari closed this Jan 31, 2023
@lhotari lhotari reopened this Jan 31, 2023
@pgier pgier force-pushed the pulsar-perf-cli-refactor branch from 3f01ce1 to 6fb9cc8 Compare February 1, 2023 16:45
@pgier
Copy link
Contributor Author

pgier commented Feb 1, 2023

Rebased on latest master to re-run CI.

@pgier pgier force-pushed the pulsar-perf-cli-refactor branch from 6fb9cc8 to 44f676d Compare February 17, 2023 16:21
@pgier
Copy link
Contributor Author

pgier commented Feb 17, 2023

Rebased on latest master again

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Mar 20, 2023
@pgier pgier force-pushed the pulsar-perf-cli-refactor branch 2 times, most recently from fa77970 to ceeb6d6 Compare May 22, 2023 14:48
* reduce code duplication between perf clients
* move common topic args and validation into new class PerformanceTopicListArguments
* improve CLI arg validation
* deprecate inconsistent args

Signed-off-by: Paul Gier <paul.gier@datastax.com>
@pgier pgier force-pushed the pulsar-perf-cli-refactor branch from ceeb6d6 to 3e52d72 Compare May 22, 2023 14:51
@lhotari lhotari added this to the 3.1.0 milestone May 22, 2023
@michaeljmarshall
Copy link
Member

/pulsarbot rerun-failure-checks

@nicoloboschi nicoloboschi merged commit 548fd22 into apache:master May 22, 2023
@lhotari lhotari added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs Stale type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants