Skip to content

Go: batch - add commands #4005

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

Merged
merged 18 commits into from
May 29, 2025
Merged

Go: batch - add commands #4005

merged 18 commits into from
May 29, 2025

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented May 27, 2025

Issue link

This Pull Request is linked to issue (URL): #3547, #2791, #3484

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.Signed-off-by: Yury-Fridlyand [email protected]

Yury-Fridlyand and others added 16 commits May 21, 2025 15:29
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Edward Liang <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner May 27, 2025 21:39
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label May 27, 2025
@Yury-Fridlyand Yury-Fridlyand changed the title BAAAAAAAAAAAAAAAAAAAAAATCH Go: batch - add commands May 27, 2025
Copy link
Collaborator

@yipin-chen yipin-chen left a comment

Choose a reason for hiding this comment

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

LGTM

@yipin-chen yipin-chen requested a review from jbrinkman May 28, 2025 00:46
Copy link
Collaborator

@jbrinkman jbrinkman left a comment

Choose a reason for hiding this comment

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

Still working to finish command reviews

//
// Command Response:
//
// The returned value for the custom command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Command Response docs seem a little misleading. Each command returns *T. It is the executeBatch that returns values. Maybe there should be a preamble like "Command Responses are returned when the Batch is executed."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should apply this change to all clients - we can do this in another PR.

Copy link
Collaborator

@jbrinkman jbrinkman left a comment

Choose a reason for hiding this comment

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

:shipit:

@jbrinkman jbrinkman self-requested a review May 28, 2025 20:12
Copy link
Collaborator

@jbrinkman jbrinkman left a comment

Choose a reason for hiding this comment

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

Looks like ZInterStore also changes parameters and drops the ZinterStoreWithOptions command

// [valkey.io]: https://valkey.io/commands/zunion/
func (b *BaseBatch[T]) ZUnionWithScores(
keysOrWeightedKeys options.KeysOrWeightedKeys,
zUnionOptions options.ZUnionOptions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The baseClient implementation has zUnionOptions as a pointer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is an error on client side actually, I created a task to fix this.

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand merged commit 4c34759 into main May 29, 2025
17 of 18 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the go/yuryf-transaction-cmds branch May 29, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go golang wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants