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

[feature] [option] adding a func as a option to randomize each request #236

Merged
merged 3 commits into from
Oct 25, 2020

Conversation

nammn
Copy link
Contributor

@nammn nammn commented Oct 22, 2020

This PR adds the ability to supply a changeFunc function for every request to ensure we can randomize data.
Solves: #197

I am using this as an internal library, before adding more e.g. to the CLI as well as updating the documentation, I wanted to clarify whether this solution makes sense implementation wise and just have a discussion in general 👍

@nammn nammn force-pushed the nnguyen/add-randomfunction branch 2 times, most recently from 5ca02db to d75ae24 Compare October 22, 2020 09:50
add some tests and todo and change signature
@nammn nammn force-pushed the nnguyen/add-randomfunction branch from d75ae24 to a7e59dd Compare October 22, 2020 09:51
@bojand
Copy link
Owner

bojand commented Oct 23, 2020

Hello, thanks for the PR!

The current package API really came more about historically as the CLI API was adapted to expose something that can be used programatically. I think in some ways there is room for improvement from package API perspective. I agree a more generalized functional API to provide data (and metadata) would be useful. It's something I've thought about for a while, and ideally really most of the other parameter options should just result in a functional "data provider", something as conceptualized here. So this is a good start I think!

One comment I have is that perhaps it would be beneficial to expose callTemplateData data publicly perhaps, and the function signature could be something like:

dataFunc func(mtd *desc.MethodDescriptor, callData *ghz.CallTemplateData) []byte

This would allow for clients to utilize the call data, as it may be helpful to have it:

// not sure what is better order of params, descriptor first and then data, or other way around?
func changeFunc(mtd *desc.MethodDescriptor, callData *ghz.CallTemplateData) []byte {
	msg := &helloworld.HelloRequest{}
	msg.Name = callData.WorkerID // now we can use call data
	binData, _ := proto.Marshal(msg)
	return binData
}

Also just thinking some more, maybe the exposed struct name should be CallData perhaps... That might be more appropriate, rather than CallTemplateData, but I don't feel too strongly.

Thoughts on all of this?

Thanks again for the PR, this looks like it should be beneficial!

@nammn
Copy link
Contributor Author

nammn commented Oct 23, 2020

That sounds great and happy to hear that. Yes, both suggestions make sense. Let me add them 👍

I think that could potentially include a bigger documentation change as this means that callTemplateData can also be used for binary requests. What do you think? Can we add this PR without that documentation change?

I planned to make this only used for the go package, does this work for you? Or do you plan to have this as a CLI Option as well? 🤔

@bojand
Copy link
Owner

bojand commented Oct 23, 2020

Hello, I am not sure what documentation changes you're referring to, it might be ambiguous. Sorry maybe I caused more confusion than necessary with some of my comments heh. All I was suggesting is:

  • Rename callTemplateData struct to CallData. With respect to code, this should result in changes only to call_template_data.go, and worker.go files. If you're using VSCode and official Go extension, "Rename Symbol" action in menu seems to do a fine job with this. You do not have to bother with fixing documentation if you do not wish like the markdown files that reference the old struct name. No problem I can address updating the docs for that.
  • Apply the new publicly exposed CallData struct to your proposed changes so that the data function has signature:
dataFunc func(mtd *desc.MethodDescriptor, callData *runner.CallData) []byte

Do not bother trying to document anything extra besides the godoc you already have. Also don't try to somehow expose this via CLI.

I think these changes I propose here are reasonable and should be sufficient for the functionality you need? Maybe I misunderstood something.

@nammn nammn force-pushed the nnguyen/add-randomfunction branch from 49e807e to 3d23afa Compare October 25, 2020 15:09
@nammn
Copy link
Contributor Author

nammn commented Oct 25, 2020

I did both changes. Checks are passing. Ready for a review 👍

@bojand bojand merged commit 10117a4 into bojand:master Oct 25, 2020
@bojand
Copy link
Owner

bojand commented Oct 25, 2020

Thanks again for the PR! I made a few cleanups, mainly added BinaryDataFunc type declaration for the data function. This has been released in 0.62.0. Feel free to open issues or further PRs if problems come up.

@nammn nammn deleted the nnguyen/add-randomfunction branch October 26, 2020 11:23
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