rust-memcache icon indicating copy to clipboard operation
rust-memcache copied to clipboard

change type for exptime param. this changes match with doc. resolve `2038` problem

Open ArtemIsmagilov opened this issue 1 year ago • 6 comments

I wanted to check with you whether you are considering support for asynchronous calls? Also, do you have any plans to fully support memcached in accordance with their protocol?

ArtemIsmagilov avatar Oct 05 '24 11:10 ArtemIsmagilov

Hi, I took a look at Memcache's protocol. Both its ASCII protocol^1 and binary protocol^2 require a 32-bit length for the expiration time. So, I think we should wait until Memcached supports a larger length.

For asyncio support (like Tokio support), I'd like to support it. However, supporting it will involve a very large changeset to this codebase. Since I don't have enough time these days, and there is already Shopify's async-memcached crate available, I'd like to mention that if someone provides a PR with high quality, I will try to merge it with glad. For current users, I recommend using async-memcached.

Also, do you have any plans to fully support memcached in accordance with their protocol?

Can you provide me some details about this?

aisk avatar Oct 05 '24 12:10 aisk

Thank you very much for your answer. When talking about full support for the memcached protocol, I mean the following - change all the inappropriate things.

Here are some examples.

  • The binary protocol has officially ceased to exist and its presence in new versions of your client will only be a suffering. For example, changes only in ASCII for some reason still pull the requirement for changes in binary

  • your flush command is used with and without nodelay. I think it would be cool to make one flush method in accordance with the protocol with an optional argument nodey (as in the protocol, although this is not so obvious). Otherwise, we will need to implement all the command options with nodely.

  • add a check for the length of the value, which should not be more than 1 megabyte. You have one for the key. Agree, it is better to validate in advance than to check on the server.

  • add all the missing gat gats and etc... commands in compliance with all the parameters from the documentation from the author of memcached

  • the SASL authentication method also needs to be updated. Command

I guess that's what I meant.

Many libraries still use basic commands. I would like to use both dsl and core commands.

ArtemIsmagilov avatar Oct 05 '24 12:10 ArtemIsmagilov

Thank you for your reply!

The binary protocol has officially ceased to exist and its presence in new versions of your client will only be a suffering. For example, changes only in ASCII for some reason still pull the requirement for changes in binary

I think maybe we can change to using ASCII protocol by default, but the binary protocol support should be kept for backword compatibility.

your flush command is used with and without nodelay. I think it would be cool to make one flush method in accordance with the protocol with an optional argument nodey (as in the protocol, although this is not so obvious). Otherwise, we will need to implement all the command options with nodely.

Rust don't have a optional argument, so there are two functions to flush databases.

add a check for the length of the value, which should not be more than 1 megabyte. You have one for the key. Agree, it is better to validate in advance than to check on the server.

I think this is a good point, check the value length in client will reduce network cost of the value is huge.

add all the missing gat gats and etc... commands in compliance with all the parameters from the documentation from the author of memcached

These commands should be supported, and PRs are welcomed!

the SASL authentication method also needs to be updated. Command

This is not SASL authentication, but memcached's own authority mechanism, and we already implmented it: https://github.com/aisk/rust-memcache/blob/14a25138485ed29f86e471bce4a22d1d98c70286/src/protocol/ascii.rs#L128-L131

aisk avatar Oct 05 '24 13:10 aisk

Thank you for commenting on my suggestions.

I might be missing something. I opened the protocol and saw the only authentication method is through Sasl There are no others.

To make sure that there is definitely no set auth command for the ASCII protocol, I manually entered the command and got ERROR. I guess I expected this. I can assume that you are using a binary protocol.

Regarding positional arguments. You can use Option parameters and, if None, simply omit the noreply flag. This should work.

For example, it would be like this: flush(norely: Option(bool)))

ArtemIsmagilov avatar Oct 05 '24 16:10 ArtemIsmagilov

I might be missing something. I opened the protocol and saw the only authentication method is through Sasl There are no others.

The link you mentioned here is not SASL. See https://github.com/memcached/memcached/wiki/SASLAuthProtocol

Regarding positional arguments. You can use Option parameters and, if None, simply omit the noreply flag. This should work.

This should work, but flush is just a simple API and I think it's not such a big deal to use either two functions or one function with an option. And we should avoid changing it for the risk of breaking current users' code.

aisk avatar Oct 07 '24 14:10 aisk

Thank you very much for correcting and sharing your views on feature development. I need to take a closer look at memcached.

ArtemIsmagilov avatar Oct 07 '24 16:10 ArtemIsmagilov