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

Go: Fix XRange and XRevRange return data type #3052

Merged
merged 4 commits into from
Feb 6, 2025

Conversation

edlng
Copy link
Contributor

@edlng edlng commented Feb 3, 2025

Issue link

This Pull Request is linked to issue (URL):

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
  • Create merge commit if merging release branch into main, squash otherwise.

edlng added 2 commits February 3, 2025 13:45
Signed-off-by: Edward Liang <[email protected]>
@edlng edlng requested a review from a team as a code owner February 3, 2025 21:59
@edlng edlng closed this Feb 3, 2025
@edlng edlng reopened this Feb 3, 2025
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label Feb 3, 2025
Comment on lines +884 to +886
sort.Slice(xRangeResponseArray, func(i, j int) bool {
return xRangeResponseArray[i].StreamId < xRangeResponseArray[j].StreamId
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we need sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think since we get a map from the CommandResponse, the order of the entries are not guaranteed. Followed by that, the iteration I do to add these map entries to an array also would not guarantee the order is maintained. So I decided to add a sort to ensure the returned array is in order

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure that we need it, but I don't mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think adding sort would be helpful in the XRevRange results but let me know your thoughts @jbrinkman

Copy link
Collaborator

Choose a reason for hiding this comment

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

This sounds like there is a problem with the Glide Core implementation. I think using sort here is warranted to guarantee that the sorting is maintained.

Comment on lines +929 to +931
sort.Slice(xRangeResponseArray, func(i, j int) bool {
return xRangeResponseArray[i].StreamId > xRangeResponseArray[j].StreamId
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reasoning as above but this time we return the order in reverse

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine.

go/api/base_client.go Show resolved Hide resolved
Signed-off-by: Edward Liang <[email protected]>
@Yury-Fridlyand Yury-Fridlyand changed the base branch from main to release-1.3 February 3, 2025 23:31
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.

Please resolve the discussions with Joe and Yury. The rest is LGTM.

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.

LG. Go ahead and merge.

Comment on lines +884 to +886
sort.Slice(xRangeResponseArray, func(i, j int) bool {
return xRangeResponseArray[i].StreamId < xRangeResponseArray[j].StreamId
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sounds like there is a problem with the Glide Core implementation. I think using sort here is warranted to guarantee that the sorting is maintained.

Comment on lines +929 to +931
sort.Slice(xRangeResponseArray, func(i, j int) bool {
return xRangeResponseArray[i].StreamId > xRangeResponseArray[j].StreamId
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine.

@yipin-chen yipin-chen merged commit 9cf4cba into valkey-io:release-1.3 Feb 6, 2025
18 checks passed
@yipin-chen yipin-chen deleted the go/xrange-xrevrange-fix branch February 6, 2025 20:29
@edlng edlng restored the go/xrange-xrevrange-fix branch February 6, 2025 21:04
@edlng edlng deleted the go/xrange-xrevrange-fix branch February 6, 2025 21:20
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.

4 participants