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

Allow consumers to set flags on keys #146

Open
Shahab96 opened this issue Apr 5, 2024 · 3 comments
Open

Allow consumers to set flags on keys #146

Shahab96 opened this issue Apr 5, 2024 · 3 comments

Comments

@Shahab96
Copy link
Contributor

Shahab96 commented Apr 5, 2024

Currently consumers have no way to set flags when writing to memcached. We should allow them to do so, however this would require changes in ProtocolTrait which will end up being a breaking change for current consumers. On the upside, if we're making a breaking change we have the opportunity to bring in async as well, which would also resolve #20 (not by using tokio specifically but by supporting async operations. Consumers can choose what async runtime they want to use)

@aisk
Copy link
Owner

aisk commented Apr 6, 2024

I think we can implement this without change ProtocolTrait. Currently the set method accept a ToMemcacheValue as the value to store to memcached. The ToMemcacheValue trait have a method named get_flags, which will be used as the flag to memcached.

And we only implmented some basic types, like i32, String as a ToMemcacheValue, so users can't change the flags easily, but they can implement their own ToMemcacheValue to override it.

However, this need users to write a lot of boilerplate codes, therefor, I think we can add a builtin struct with implement this trait, and which hold a flags and other informations, this can be used for users, also can be used to implement #143.

@Shahab96
Copy link
Contributor Author

Shahab96 commented Apr 6, 2024

Interesting idea. I'm unsure how we'd implement this as I'm still getting used to this codebase but I'll do some investigation today and see what I can come up with

@Shahab96
Copy link
Contributor Author

Shahab96 commented Apr 7, 2024

So, looking at the code again, I don't see how we would be able to do this without having consumers implement ToMemcacheValue explicitly. Additionally, it doesn't seem like a very intuitive interface when implementing the trait for some struct, as now you are able to call set and provide the expiration, but not flags as arguments. This becomes a divergence in the interface where some memcached arguments are part of the set function and others are to be implemented by the consumer. At the very least we can provide a more expansive set of functions and put them behind a feature flag. The interface as a whole doesn't seem to comply with the memcached api and imo it seems like we're working around an interface that was incompletely designed in order to avoid breaking changes.

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

No branches or pull requests

2 participants