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

Update the message size when a pattern is provided via -m #1899

Merged
merged 1 commit into from
Aug 15, 2018

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Jul 26, 2018

I wanted to use the performance tool to stress our application, which requires messages with specific content (JSON, authorization parts, types, ...). Using -m to specify the JSON was easy, but I found it very counter-intuitive that it would repeat the pattern/cut it off unless I explicitly also specified the length.

This PR changes this behavior so that the length gets adjusted by default when -m is used. A user that wants cut-off/repeat can specify the length as before by setting it after the -m switch.

For my purposes this is enough, but it would break uses where the caller runs rdkafka_performance -s SIZE -m PATTERN. One approach for avoiding this breakage would be track whether the user set the size explicitly, and only update in -m when that is not the case. Thoughts?

@mhowlett
Copy link
Contributor

thanks @ankon - i do like the idea of size automatically being set to the message pattern length in the case it's not specified (implementation as you suggest). i don't think it's a good idea for behavior to depend on order in which flags are specified though - it's arguably manageable for just one option, but allowing this sort of thing would quickly get out of hand.

@ankon
Copy link
Contributor Author

ankon commented Jul 30, 2018

One idea could be to have a different option for "size-of-message-with-repeating". The other idea would be exactly the opposite: Keep -m to mean pattern (and therefore -s to mean repeated-size), and add -M or such to mean "complete message". The more I think about it, the more I like the latter -- it wouldn't break any existing code, and just make the life easier for new users.

@mhowlett
Copy link
Contributor

I think I agree with you. I think the interface is better with just two arguments, but your 3 argument suggestion seems very reasonable and as you point out avoids breaking anything (which would seem an important consideration here). Would suggest revising the PR (and @edenhill will review when he's available).

@ankon
Copy link
Contributor Author

ankon commented Jul 31, 2018

The tool is seriously running out of option characters ... :)

I randomly picked -e now, as that was the first free one, but cannot yet come up with a good name for that.

I'm also looking a bit into the defaulting logic: Ideally the existing uses all stay what they mean, so the logic would be: "if -e is given we set the message pattern to the value, and adjust the size."

What is a bit interesting is conflict handling: "If a -s or -m is given before that -e, the -e wins. If -s or -m is given after -e, then these win." My argument here would be that someone providing both -e and -s/-m should know what they are doing, since that -e is a new option.
An alternative approach could be to abort with an error if -s/-m is given together with -e.

@edenhill
Copy link
Contributor

edenhill commented Aug 6, 2018

I think the most intuitive approach is to let the message size be strlen(-m), unless it was explicitly set with -s. And yes, it should be possible to set -s before -m.

When -s is > -m, repeat -m pattern.
When -s is < -m, truncate -m.

@ankon ankon force-pushed the pr/perf-recalc-message-size branch from 0cf47a8 to c4c6fac Compare August 9, 2018 11:01
@ankon
Copy link
Contributor Author

ankon commented Aug 9, 2018

I think the most intuitive approach is to let the message size be strlen(-m), unless it was explicitly set with -s. And yes, it should be possible to set -s before -m.

Works for me! :)

@edenhill edenhill merged commit 175463d into confluentinc:master Aug 15, 2018
@edenhill
Copy link
Contributor

Great stuff!

@ankon ankon deleted the pr/perf-recalc-message-size branch August 15, 2018 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants