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

Add binary support #172

Merged

Conversation

avifenesh
Copy link
Member

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@avifenesh avifenesh force-pushed the cluster_scan/bunary_support branch 2 times, most recently from 25060da to 4622d98 Compare July 7, 2024 12:34
@avifenesh avifenesh requested a review from eifrah-aws July 7, 2024 12:37
@avifenesh avifenesh force-pushed the cluster_scan/bunary_support branch from 4622d98 to a5436e1 Compare July 7, 2024 12:44
@avifenesh avifenesh changed the title added blocking commands tests Add binary support Jul 7, 2024
@@ -139,16 +139,16 @@ where
})
}

// Special handling for `SCAN` command, using cluster_scan
/// Special handling for `SCAN` command, using cluster_scan.

Choose a reason for hiding this comment

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

nit: using `cluster_scan`

@@ -487,7 +487,7 @@ where
async fn send_scan<C>(
scan_state: &ScanState,
core: &C,
match_pattern: Option<String>,
match_pattern: Option<Vec<u8>>,

Choose a reason for hiding this comment

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

BytesMut is more efficient, any special reason why you choose Vec<u8> ? (BytesMut is similar to Vec<u8> but with less copies and allocations)

Copy link
Member Author

Choose a reason for hiding this comment

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

The built in function of the library is to_redis_args which return Vec

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be better to refactor but not at this point

Choose a reason for hiding this comment

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

Makes sense not to make a drastic change at this point

@avifenesh avifenesh force-pushed the cluster_scan/bunary_support branch from a5436e1 to 789f324 Compare July 7, 2024 13:09
@eifrah-aws eifrah-aws self-requested a review July 7, 2024 13:18
@avifenesh avifenesh merged commit ca36bd4 into amazon-contributing:main Jul 7, 2024
10 checks passed
@avifenesh avifenesh deleted the cluster_scan/bunary_support branch July 7, 2024 14:13
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