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

Producer performance tool #1222

Merged
merged 15 commits into from
Feb 7, 2019
Merged

Producer performance tool #1222

merged 15 commits into from
Feb 7, 2019

Conversation

thesalmontapes
Copy link
Contributor

@thesalmontapes thesalmontapes commented Nov 22, 2018

This tool mirrors kafka-producer-perf-test.sh in the Kafka distribution. I was using this myself for some performance testing on this library and thought I might as well drop this here in in case others find it useful.

@ghost ghost added the cla-needed label Nov 22, 2018
@varun06
Copy link
Contributor

varun06 commented Nov 24, 2018

Looks great. 👍

@ghost ghost removed the cla-needed label Nov 27, 2018
tools/README.md Outdated Show resolved Hide resolved
default:
printUsageErrorAndExit(fmt.Sprintf("Unknown -compression: %s", scheme))
}
panic("should not happen")
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason to use panic, what about log.Fatal("should not happen").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is programming error and panic prints a stack trace; if we hit this line, there's clearly a bug since we would have expected to exit on line 136.

}
}()

messagesDone := make(chan struct{})
Copy link
Contributor

Choose a reason for hiding this comment

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

you can pass context to these function and then exit on context.Done() from go routine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean passing the context from the main routine to runAsyncProducer or instantiating a new context in this function and replacing messagesDone with it? The first case is not desirable because we don't want to exit from this function until this goroutine has finished (i.e. all the messages have been successfully sent, or an error occurred). The second case is really just a matter of style; we can wait on context.Done at the end of the function or this messagesDone channel. I don't have a huge preference here.

@varun06
Copy link
Contributor

varun06 commented Jan 16, 2019

This can be helpful, can we get it going?

@varun06
Copy link
Contributor

varun06 commented Feb 7, 2019

@thesalmontapes should I merge it?

@thesalmontapes
Copy link
Contributor Author

thesalmontapes commented Feb 7, 2019

By all means!

@varun06 varun06 merged commit 8e19158 into IBM:master Feb 7, 2019
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.

2 participants