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

[pulsar-perf] Make it possible to disable poolMessages #12090

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Sep 18, 2021

Motivation

In Pulsar 2.8.0 with pulsar-perf, I hit the bug #11824 where the pulsar-perf consume stops consuming some partitions.

As a workaround, I tried to pass -pm false or --pool-messages false on the command line, but this resulted in error message:
The size of topics list should be equal to --num-topics

The problem is that JCommander doesn't expect arguments to boolean parameters by default.
This is explained in the JCommander manual and issue comment cbeust/jcommander#129 (comment) .

Modifications

  • add arity = 1 to the JCommander Parameter annotation.
    • JCommander requires specifying arity = 1 to boolean parameters that have a default value of true.

- JCommander requires passing arity = 1 to boolean parameters that
  have a default value of true.
@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug release/2.8.2 labels Sep 18, 2021
@lhotari lhotari added this to the 2.9.0 milestone Sep 18, 2021
@lhotari lhotari self-assigned this Sep 18, 2021
@yuruguo
Copy link
Contributor

yuruguo commented Sep 18, 2021

LGTM. Should we also cover other similar parameters?

@Parameter(names = {"--batch-index-ack" }, description = "Enable or disable the batch index acknowledgment")
public boolean batchIndexAck = false;

@Parameter(names = { "-pm", "--pool-messages" }, description = "Use the pooled message")
@Parameter(names = { "-pm", "--pool-messages" }, description = "Use the pooled message", arity = 1)
Copy link
Contributor

@yuruguo yuruguo Sep 18, 2021

Choose a reason for hiding this comment

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

Good catch!Should other parameter that type is boolean be handled like this? such as --replicated, --batch-index-ack, --auto_ack_chunk_q_full etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be a breaking change. This is only necessary for boolean options that default to true.

@lhotari
Copy link
Member Author

lhotari commented Sep 18, 2021

LGTM. Should we also cover other similar parameters?

It would be a breaking change. This is only necessary for boolean options that default to true.

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

@merlimat merlimat merged commit 898582b into apache:master Sep 20, 2021
codelipenghui pushed a commit that referenced this pull request Sep 24, 2021
- JCommander requires passing arity = 1 to boolean parameters that
  have a default value of true.

(cherry picked from commit 898582b)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 24, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
- JCommander requires passing arity = 1 to boolean parameters that
  have a default value of true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.2 release/2.9.0 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants