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.

Sign up for GitHub

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

Jump to bottom

GO Implement Dump, Restore and ObjectEncoding Command #2781

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

GO Implement Dump, Restore and ObjectEncoding Command #2781

wants to merge 13 commits into from
+302 −0

Conversation

EdricCua
Copy link
Collaborator

@EdricCua EdricCua commented Dec 3, 2024 •
edited
Loading

Issue link

This Pull Request is linked to issue (URL): https://github.com/orgs/valkey-io/projects/10/views/10?pane=issue&itemId=91968108&issue=valkey-io%7Cvalkey-glide%7C2853

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.

@EdricCua EdricCua requested a review from a team as a code owner December 3, 2024 09:23
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label Dec 3, 2024
Yury-Fridlyand
Yury-Fridlyand previously requested changes Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

I recommend implementing dump together with restore so you can test it and validate that both work. Otherwise dump is untested.

go/api/generic_commands.go Outdated Show resolved Hide resolved
@EdricCua EdricCua changed the title GO Implement Dump and ObjectEncoding Command GO Implement Dump, Restore and ObjectEncoding Command Dec 5, 2024
@EdricCua
Add restore command
7b4710b 
Signed-off-by: EdricCua <[email protected]>
@Yury-Fridlyand Yury-Fridlyand self-requested a review December 6, 2024 18:21
@Yury-Fridlyand Yury-Fridlyand dismissed their stale review December 6, 2024 18:22

Restore added

Yury-Fridlyand
Yury-Fridlyand reviewed Dec 27, 2024
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Few small comments left.
Since dump-restore cycle works fine we can confirm that go client support binary strings. That's really good news.

go/api/command_options.go Outdated Show resolved Hide resolved
go/api/base_client.go Outdated Show resolved Hide resolved
go/api/generic_commands.go Outdated Show resolved Hide resolved
go/api/generic_commands.go Outdated Show resolved Hide resolved
go/api/generic_commands.go Outdated Show resolved Hide resolved
go/integTest/shared_commands_test.go Outdated Show resolved Hide resolved
@prateek-kumar-improving
Copy link
Collaborator

Please update PR description checklist and add the linked issue for this PR.

@prateek-kumar-improving
Copy link
Collaborator

Please update changelog.md file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prateek-kumar-improving prateek-kumar-improving prateek-kumar-improving left review comments

@yipin-chen yipin-chen yipin-chen left review comments

@tjzhang-BQ tjzhang-BQ tjzhang-BQ left review comments

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
go golang wrapper
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EdricCua @prateek-kumar-improving @Yury-Fridlyand @tjzhang-BQ @yipin-chen