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 reading & writing of flags #18

Closed
wants to merge 5 commits into from
Closed

Conversation

aspett
Copy link

@aspett aspett commented Mar 6, 2019

Adds reading and writing of flags to achieve compatibility with Dalli (ruby client). Fixes #7. Takes inspiration from #8 and expands on comments.

  • Allows passing a flags option to Memcache.xxx functions that take options.
  • Supports :serialised, :compressed flags (0x1, 0x2 respectively)
  • Adds a encode_flags callback for Coders with a default passthrough impl.
  • Adds a default :serialised flag to the JSON Coder.

There are some slightly breaking changes here, but the core API (Memcache module) is not affected.

  1. Accessing Memcache.Connection.execute/4 and Memcache.Connection.execute_quiet/2 directly will give results back in a different format, potentially; {:ok, value :: any, flags :: keyword} instead of {:ok, value :: any}. I'm pretty sure it's impossible to add flag support without this change, unfortunately.
  2. Coders should now use Memcache.Coder rather than @behaviour use Memcache.Coder and optionally implement encode_flags/2.

@aspett
Copy link
Author

aspett commented Mar 7, 2019

👀 Tests passed, but builds failed?

@ananthakumaran
Copy link
Owner

ananthakumaran commented Mar 8, 2019

$ ! mix help format > /dev/null || mix format --check-formatted
** (Mix) mix format failed due to --check-formatted.
The following files were not formatted:
  * test/memcache/connection_test.exs
  * lib/memcache/receiver.ex
  * lib/memcache/connection.ex
  * lib/memcache.ex
The command "! mix help format > /dev/null || mix format --check-formatted" exited with 1.

@aspett
Copy link
Author

aspett commented Mar 12, 2019

Thanks @ananthakumaran. I didn't notice it was running that. Have mix format'd it with elixir 1.7.

@aspett
Copy link
Author

aspett commented Apr 9, 2019

Bump @ananthakumaran

@ananthakumaran
Copy link
Owner

sorry for the delay. I am kind of reluctant about merging patches that add the ability to read/write flags. There are two aspects, 1) I haven't given much thought to flags during the initial implementation which makes it hard to retrofit. 2) There aren't many use cases which require the flags. I don't want a situation where you finish the PR, but I decide not to merge because of the above reasons.

With that said, here are my thoughts.

I don't like to attach meaning to flag (aka bit 1 is serialized, bit 2 is compressed, etc). As the flag page indicates this is different across language bindings. What I would instead like is, just expose it via the coder.

encode(any, options :: Keyword.t()) :: iodata
encode(any, options :: Keyword.t()) :: {flag :: binary, iodata}
@callback decode(iodata, options :: Keyword.t()) :: any
@callback decode(iodata, flag :: binary, options :: Keyword.t()) :: any

basically, encode can return flag value and decode can accept flag,
we probably have to make some changes to support the old coder
interface. With this, a custom coder could be used to support dalli without changes anywhere else in the application.

I am torn about the changes in Memcache.Connection. I don't want to break the API, but I don't have any better suggestion either. If I had used a map/struct for the return value, it would have made things a lot easier.

@wizardone
Copy link

@ananthakumaran It seems that you are not eager to merge this PR as it is. Are you willing to reconsider it or would you like to see a different approach being taken here so we can eventually have the flags functionality in place?

@ananthakumaran
Copy link
Owner

ananthakumaran commented Jun 4, 2019 via email

@aspett
Copy link
Author

aspett commented Dec 16, 2019

Closing due to no movement.

@aspett aspett closed this Dec 16, 2019
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.

Dalli Compatibility
3 participants