Skip to content

Add meta_flags passthrough to write/delete/arithmetic and pipelined ops#70

Merged
nherson merged 4 commits intomainfrom
nherson/meta-flags-passthrough
May 5, 2026
Merged

Add meta_flags passthrough to write/delete/arithmetic and pipelined ops#70
nherson merged 4 commits intomainfrom
nherson/meta-flags-passthrough

Conversation

@nherson
Copy link
Copy Markdown

@nherson nherson commented Apr 27, 2026

Summary

The meta protocol formatter previously supported a meta_flags: kwarg only on meta_get, exposed publicly via Client#get and Client#gat. This extends the same passthrough to the rest of the operations:

  • Client#set / add / replace / cas — already accepted req_options; now plumbed through to RequestFormatter.meta_set
  • Client#append / prependreq_options added to the public signature
  • Client#delete / delete_casreq_options added
  • Client#incr / decrreq_options added as 5th positional arg
  • Client#get_multi / get_multi_cas — trailing **req_options kwargs
  • Client#set_multi — already accepted req_options; now flows down to the inline ms line in write_multi_storage_req
  • Client#delete_multireq_options added

Pipelined behavior

For pipelined operations (get_multi, set_multi, delete_multi) the same req_options hash is applied to every command in the pipeline. This is documented as best-effort: callers are responsible for ensuring the flags are appropriate for every key in the batch. Per-key flag overrides are intentionally not supported, since the obvious motivating use case — a single proxy/routing directive that should apply to every key in a batch — doesn't need them, and supporting them would significantly complicate the wire-building hot path.

Public API

dc.set('k', 'v', 60, meta_flags: ['Pfoo'])
dc.add('k', 'v', 60, meta_flags: [...])
dc.replace('k', 'v', 60, meta_flags: [...])
dc.delete('k', meta_flags: [...])
dc.append('k', 'suffix', meta_flags: [...])
dc.prepend('k', 'prefix', meta_flags: [...])
dc.incr('k', 1, 60, 0, meta_flags: [...])
dc.decr('k', 1, 60, 0, meta_flags: [...])

# Pipelined — same flags applied to every key
dc.get_multi('a', 'b', 'c', meta_flags: ['Pfoo'])
dc.get_multi_cas('a', 'b', meta_flags: [...])
dc.set_multi({ 'a' => '1', 'b' => '2' }, 60, meta_flags: [...])
dc.delete_multi(%w[a b c], meta_flags: [...])

All changes are additive and backwards-compatible; existing callers are unaffected.

Implementation notes

  • RequestFormatter.meta_set / meta_delete / meta_arithmetic gain a meta_flags: kwarg mirroring meta_get. Empty arrays are now treated as no flags everywhere (previously meta_get with meta_flags: [] would emit a stray trailing space).
  • read_multi_req and write_multi_storage_req build the wire format inline for performance, bypassing the formatter; a small build_meta_flags_suffix helper handles the splice.
  • Protocol::Base#pipelined_get and Protocol::Meta#quiet_get_request accept a req_options arg so the multi-server get_multi path also threads flags through.
  • PipelinedGetter#process and PipelinedDeleter#process accept a trailing req_options arg, with YARD comments documenting the best-effort contract.

Tests

  • test/protocol/meta/test_request_formatter.rb: +5 unit tests covering meta_flags on meta_get / meta_set / meta_delete / meta_arithmetic, plus the empty-array case.
  • test/integration/test_meta_flags_passthrough.rb (new): 10 tests exercising every public method against a real memcached, plus a negative control that asserts an unrecognized flag letter raises Dalli::DalliError (locks in the protocol's flag-validation contract).

Local results:

  • Unit tests: 44 runs, 0 failures.
  • New integration tests: 10 runs, 0 failures.
  • Full suite (excluding test_rack_session.rb, which is broken by an unrelated Ruby 4.0 / cgi/cookie issue): 345 runs, 109,964 assertions, 0 failures. The 10 errors observed are pre-existing toxiproxy ECONNREFUSED errors unrelated to this change.
  • Rubocop: clean on every touched file.

The meta protocol formatter previously supported a meta_flags: kwarg only
on meta_get, exposed publicly via Client#get and Client#gat. This extends
the same passthrough to the rest of the operations:

  * Client#set / add / replace / cas (already had req_options; now plumbed
    through to RequestFormatter.meta_set)
  * Client#append / prepend (req_options added to public signature)
  * Client#delete / delete_cas (req_options added)
  * Client#incr / decr (req_options added as 5th positional arg)
  * Client#get_multi / get_multi_cas (trailing **req_options kwargs)
  * Client#set_multi (already had req_options; now flows to ms line)
  * Client#delete_multi (req_options added)

For pipelined operations (get_multi, set_multi, delete_multi) the same
req_options hash is applied to every command in the pipeline. This is
documented as best-effort: callers are responsible for ensuring the
flags are appropriate for every key in the batch. Per-key flag overrides
are intentionally not supported.

Tests:
  * RequestFormatter unit tests cover meta_flags on get/set/delete/arith.
  * New integration suite test_meta_flags_passthrough.rb exercises every
    public method against a real memcached, including a negative control
    proving memcached rejects unrecognized flags.
@nherson nherson self-assigned this Apr 29, 2026
end
end

describe 'delete' do
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We never delete in node-local cache, just let cache entry auto expired. For delete and delete_multi, what does the PROXY_FLAGS mean? Why do we need the p and l flags for deletions?

Copy link
Copy Markdown

@drinkbeer drinkbeer left a comment

Choose a reason for hiding this comment

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

We talked about this PR, it looks good to me. 🚢

@drinkbeer
Copy link
Copy Markdown

drinkbeer commented Apr 30, 2026

The meta_flags options are supported in delete and delete_multi which make me easier to support the Tomestone to prevent dirty writes.

@danmayer
Copy link
Copy Markdown

danmayer commented May 1, 2026

Some things flagged when I checked this with claude... I think some of these are worth addressing with another quick pass.

Concerns / suggestions

  1. API inconsistency: kwargs vs positional hash.
    get_multi/get_multi_cas use *req_options, while set_multi/delete_multi/set/delete/incr/decr use a trailing positional req_options =
    nil. Mixed style means call sites read differently:
    dc.get_multi('a', 'b', meta_flags: [...]) # kwargs
    dc.delete_multi(%w[a b], meta_flags: [...]) # also looks like kwargs but is positional Hash
    dc.incr('k', 1, 60, 0, meta_flags: [...]) # positional Hash
    This is forced for get_multi
    because *keys collides with a trailing options hash, so the inconsistency is unavoidable in the public
    API. Worth a one-line note on each method's YARD that calls out the positional-vs-kwargs distinction so users don't get bitten when
    they refactor.
    get_multi/get_multi_cas use *req_options, while set_multi/delete_multi/set/delete/incr/decr use a trailing positional req_options =
    nil. Mixed style means call sites read differently:
    dc.get_multi('a', 'b', meta_flags: [...]) # kwargs
    dc.delete_multi(%w[a b], meta_flags: [...]) # also looks like kwargs but is positional Hash
    dc.incr('k', 1, 60, 0, meta_flags: [...]) # positional Hash
    This is forced for get_multi
    because *keys collides with a trailing options hash, so the inconsistency is unavoidable in the public
    API. Worth a one-line note on each method's YARD that calls out the positional-vs-kwargs distinction so users don't get bitten when
    they refactor.

  2. Latent backwards-compat risk on get_multi.
    get_multi(*keys) → get_multi(*keys, **req_options). In Ruby 3 keyword separation, callers passing a plain hash positionally
    (get_multi('a', { foo: 1 })) still go into keys — safe. But callers using get_multi('a', foo: 1) previously got {foo: 1} flattened into
    keys (and likely produced a no-op or error downstream); now it's silently absorbed as req_options. Almost certainly a no-op in
    practice but worth flagging in the changelog.

  3. meta_flag_options called repeatedly per request.
    e.g. lib/dalli/protocol/meta.rb:187 does if meta_flag_options(options) followed by another meta_flag_options(options) call further
    down. Cheap (just a Hash lookup), but on the per-op path it'd be tidier — and slightly nicer for readers — to assign once. Not
    blocking.

  4. pipelined_deleter multi-server is silently a no-op for flags.
    Pipelined::Deleter#process accepts req_options but the multi-server path is "not yet implemented for pipelined delete" (existing
    comment), so the actual multi-server branch is in Client#delete_multi → delete_cas per key. That branch does forward req_options
    correctly (client.rb:289). Worth a one-line comment on the deleter clarifying that the multi-server fallback in Client is what carries
    flags through.

  5. Test gap: empty meta_flags: [] integration.
    Unit tests cover the empty-array case at the formatter layer, but there's no integration assertion that meta_flags: [] works on
    set/delete/incr/get_multi. Cheap to add — and the empty-array fix is the one behavioral change for existing meta_get callers, so worth
    pinning down.

@nherson nherson merged commit bc77fa5 into main May 5, 2026
19 of 20 checks passed
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.

3 participants