feat: nil value allowed in message / topic optional #83
Conversation
gemantzu
force-pushed
the
fixes-non-binary-value
branch
from
fa63199 to
0ef9ddd
Compare
|
I am going to fix the test. I would also like to pick your ears on making the partition_fun non required as well, since it has a default. |
| @@ -252,7 +252,7 @@ defmodule Kafee.Producer.Message do | |||
|
|
|||
| def validate!(%Message{} = message) do | |||
| for {key, value} <- message.headers do | |||
| if not (is_binary(key) and is_binary(value)) do | |||
| if not (is_binary(key) and allowed_value?(value)) do | |||
There was a problem hiding this comment.
It's not immediately apparent why this is desirable behavior. Rather than patching Kafee to allow nil header values, can you help us better understand why we're sending those values in the first place and are unable to fix the originating code instead?
There was a problem hiding this comment.
We are not always able to have an actual value on a key. We stumbled upon this a few times on development, therefore the patch. We probably need to think this even more with additional changes, ie maybe we need to tolerate atom keys in general, or we can call to_string in the value (with or without inspect) for more complex values.
There was a problem hiding this comment.
Can't we eject nil values on the service side?
| test "raises on non string header value", %{topic: topic} do | ||
| test "does not raise on non string header value", %{topic: topic} do |
There was a problem hiding this comment.
Since we're flipping this test, it makes me think this is more of a breaking change than a chore?
There was a problem hiding this comment.
that's true. I am going to change the title.
gemantzu
changed the title
| @@ -372,4 +372,8 @@ defmodule Kafee.Producer.Message do | |||
| #{inspected_message} | |||
| """ | |||
| end | |||
|
|
|||
| defp allowed_value?(nil), do: true | |||
There was a problem hiding this comment.
nil values are technically invalid by the low level kafka protocol library used by brod and most other Erlang libraries:
We will need to test this a ton to ensure it actually works. I have a hunch that brod to_string encodes nil which just results in "".
There was a problem hiding this comment.
I wouldn't suggest allowing nil values since I can't see how they would be meaningful in metadata. We could however, delete any keys with nil values instead of raising an exception.
There was a problem hiding this comment.
It would be good to look at the code @gemantzu is referring that's having trouble with this. This feels like something that should be handled service side 🤔
| @@ -70,7 +70,7 @@ defmodule Kafee.Producer do | |||
| doc: """ | |||
| The Kafka topic to send messages to. | |||
| """, | |||
| required: true, | |||
| required: false, | |||
There was a problem hiding this comment.
This should be fine for most things, though I'd probably add some additional code to error on the async adapter as it will cause a ton of processes to spawn for every new topic sent to.
There was a problem hiding this comment.
well, we should at some point explore the possibility to make this indeed the default, as it would enforce us to have one producer (and even better, one consumer) per topic instead of the multi use ones we have at the moment, but if we want to adapt now version 3.0, this is making it impossible unless we implement those changes.
|
Slightly unrelated to this PR but to avoid running into message header validation exceptions, we can either make the typespecs of the publish function in WarehouseEvents.KafeeProducer (and the Parcel service one which is almost identical) more strict |
|
Ok. Based on the discussions, I am going to remove the PR from here and push a PR on the two services for the typespecs so that dialyzer can catch it. |
Checklist
Problem
Details