karafka
Headers.from_native rdoc says it returns Hash<String, String> but actually it is Hash<Symbol, String>
I was surprised to get headers returned as a Hash<Symbol, String> when the Headers documentation and Message documentation both say Hash<String, String>.
I'd happy to make a PR to address this.
My preference would be to change the code to match the documentation. (There's no benefit here to use Symbol keys. The code is converting every header into a Symbol for each message consumed... but Symbol's benefit of O(1) comparision time will only help when a header is explicitly checked by the application...which is likely a small fraction of the time. And that is probably wasted anyway because the headers are stored in a Hash and that achieves a similar optimization for String keys, by hashing them.) Of course switching the code to match the docs would be a breaking change so it would have to be released with the next major version number bump.
If you prefer Symbol keys as implemented, I could create a PR that just fixes the docs in those two places.
A third choice would be to add a config variable for preference of Symbol or String header keys. That would default to Symbol to match current behavior, but teams like us that prefer String could change the config, and the default could switch at the next major version bump. I'd be fine making this PR.
Also, as a nit, I noticed that class Headers isn't really a class--it's just a module (namespace) wrapping the from_native method. Ok to include that correction in whichever PR above?
Yeah ref https://github.com/appsignal/rdkafka-ruby/blob/main/lib/rdkafka/consumer/headers.rb#L54=
I would opt on the approach @ColinDKelley suggests as well to normalize it to be string only.
actually this card is a duplicate of one that I created while ago: https://github.com/appsignal/rdkafka-ruby/issues/168
Great, I'm happy to make a PR that changes the keys to strings including in the specs.
I'm fine closing this ticket out as a duplicate of #168. But I have this question remaining first:
Assuming we follow Sem Ver, that would indicate we need a major version bump to release this fix, since it will break any code looking up message headers by Symbol. Any objections to that? Or anyone feel it's not necessary? Of course that major version bump could be grouped with other breaking changes if there are any pending.
Update: I just noticed that the current version is 0.12. Sem Ver has an exemption that interfaces starting with 0. aren't yet deemed stable so they're allowed to make breaking changes without bumping the major version.
@thijsc Now that this PR is merged into master, can you please release a new version sometime soon that includes it?