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

feat(services/memcached): change to binary protocal #4252

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Conversation

hoslo
Copy link
Contributor

@hoslo hoslo commented Feb 23, 2024

Part #4177

@Xuanwo Basically done, do you have any suggestions for users to use different protocols to access memcache? sasl is only available in bianry.

core/Cargo.toml Outdated
@@ -56,6 +56,7 @@ default = [
"services-webdav",
"services-webhdfs",
"services-azfile",
"services-memcached",
Copy link
Member

Choose a reason for hiding this comment

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

Please don't add services in default set.

core/src/services/memcached/backend.rs Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2024

do you have any suggestions for users to use different protocols to access memcache?

I'm not certain, but it seems like binary consistently outperforms ascii, doesn't it? Do you have a basic bench about them?

@hoslo
Copy link
Contributor Author

hoslo commented Feb 23, 2024

do you have any suggestions for users to use different protocols to access memcache?

I'm not certain, but it seems like binary consistently outperforms ascii, doesn't it? Do you have a basic bench about them?

https://www.memcachier.com/documentation/supported-protocols-ascii-binary.
It seems that the binary protocol is a bit faster than the ascii protocol and supports authentication.

@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2024

It seems that the binary protocol is a bit faster than the ascii protocol and supports authentication.

That's great! We can remove ascii support.

@hoslo hoslo changed the title feat(services/memcached): add binary protocal support feat(services/memcached): change to binary protocal Feb 23, 2024
@hoslo hoslo force-pushed the add-ascii-auth branch 2 times, most recently from 68333ba to f483939 Compare February 23, 2024 04:51
@hoslo hoslo marked this pull request as ready for review February 23, 2024 04:53
@hoslo hoslo requested a review from PsiACE as a code owner February 23, 2024 04:53
CHANGELOG.md Outdated Show resolved Hide resolved
@hoslo hoslo force-pushed the add-ascii-auth branch 3 times, most recently from 8ca86e7 to 2de5194 Compare February 23, 2024 06:59
@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2024

Would you like to add a test for memcached with password? And do we need sasl at client side?

@hoslo
Copy link
Contributor Author

hoslo commented Feb 23, 2024

Would you like to add a test for memcached with password? And do we need sasl at client side?

Yes, I will add it in another pr, we do not need sasl at client side.

@Xuanwo
Copy link
Member

Xuanwo commented Feb 23, 2024

Yes, I will add it in another pr, we do not need sasl at client side.

Got it. Let's move!

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo Xuanwo merged commit 2f85fa3 into main Feb 23, 2024
53 checks passed
@Xuanwo Xuanwo deleted the add-ascii-auth branch February 23, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants