-
Notifications
You must be signed in to change notification settings - Fork 68
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
Go: Implement Custom Command with route(Generic Cluster cmd), Ping with Route #2979
base: main
Are you sure you want to change the base?
Go: Implement Custom Command with route(Generic Cluster cmd), Ping with Route #2979
Conversation
Signed-off-by: Niharika Bhavaraju <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed API naming with the team and agreed to follow approach we started with node client. If command have optional argument, we wrap them with options
struct and provide 2 APIs: cmd
and cmdWithOptions
.
Currently PING has 2 optional arguments and 4 APIs (4 permutations):
Ping
PingWithMessage
PingWithRoute
PingWithMessageAndRoute
Imagine PING
gets a new optional argument later in future. Current solution is not scalable. So the proposal is to refactor it now into the following:
Ping
PingWithOptions
And follow that approach for all commands.
If you have any concerns, please, share. I'm happy to discuss that there or in a GLIDE contribution meeting.
// the executed | ||
// command. | ||
// | ||
// The command will be routed automatically based on the passed command's default request policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line
// the executed | ||
// command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the executed | |
// command. | |
// the executed command. |
// result, err := client.PingWithMessage("Hello", route) | ||
// fmt.Println(result) // Output: "Hello" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// result, err := client.PingWithMessage("Hello", route) | |
// fmt.Println(result) // Output: "Hello" | |
// result, err := client.PingWithMessage("Hello", route) | |
// fmt.Println(result) // Output: "Hello" |
// result, err := client.Ping(route) | ||
// fmt.Println(result) // Output: "PONG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// result, err := client.Ping(route) | |
// fmt.Println(result) // Output: "PONG" | |
// result, err := client.Ping(route) | |
// fmt.Println(result) // Output: "PONG" |
assert.Nil(suite.T(), err) | ||
value := result.Value() | ||
|
||
fmt.Printf("Value type: %T\n", value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
Go: Implement Custom Command with route(Generic Cluster cmd), Ping with Route