opened
Leonidas-from-XIV
username_0
I was wondering how to specify unassigned partition in the produce functions and only then realized that there is a special sentinel value that will be picked. This feels very C-ish and non-discoverable since `-1` is in the same value space as `int`.

Another downside is that the constant is `-1` in OCaml, whereas the constant is `RD_KAFKA_PARTITION_UA` on the rdkafka side, which is probably `-1` but that's a somewhat fragile setup.

So instead I decided to change the `partition` type, but then I ran into all kinds of issues with terrible APIs since whenever `partitions` are returned they are always "Assigned" so code was forced to handle `Unassigned` cases where they could never happen.

A much better approach is to use a special type when it is possible to not assign a partition, `partition_assignment`.

There's different ways to do that:

1. `type partition_assignment = Assigned of int | Unassigned` is very simple, but using it is kinda noisy (`Kafka.Assigned 42`) and the type itself does not feel very useful. This is the current PR but I am more than willing to change it.
2. ``type partition_assignment = `Assigned of int | `Unassigned`` is somehow better because then the user can just make up `` `Assigned 42`` and does not need the constructor names. I have seen that used in Jane Street code and I think it is a neat pattern.
3. `type partition_assignment = int option` is certainly the shortest one since option types are very common but `None` is a somewhat obscure way to spell `Unassigned`. Though this would allow to make the `partition` argument be optional, which could be a nice API.

Please let me know what you think, I'll gladly change the current PR since the required changes in any direction are rather minimal.
Change signature of Kafka partitions
issue
created
Leonidas-from-XIV
username_0
On a related note, @username_1 maybe you could disable and re-enable the Travis CI integration on this repo to try to get the colorful CI bubbles back? It seems Travis has changed something and it stopped working some time ago.
comment
created
didier-wenzek
username_1
Thank you for this PR.

I agree that having the value `partition_unassigned` defined after the actual value of a C constant is fragile, even if unlikely to be broken in the future. As you stress it, this is also not easy to discover that a message can be produced with no particular partition assigned.

Hence, I agree to change the API.

Among all the alternatives you propose, I prefer the 3rd one with an optional argument.

```OCaml
val produce: topic -> ?partition:partition -> ?key:string -> ?msg_id:msg_id -> string -> unit
```

I believe that with an optional argument the usage of `None` for `Unassigned` will not be confusing.

I also wonder if the partition callbacks have to return an actual partition or have the option to let it unassigned. I have to make some tests, here.
comment
created
Leonidas-from-XIV
username_0
It would be good if you could test this. Meanwhile I changed the code to use `int option` instead. To my bemusement, no changes in the C code were necessary since the `partition_assignment` and `int option` are represented in the exact same way in memory.
comment
created
didier-wenzek
username_1
According to the [documentation of librdkafka](https://github.com/edenhill/librdkafka/blob/master/src/rdkafka.h#L2403), a partitioner callback can return the `RD_KAFKA_PARTITION_UA` value. Hence, the signature [`?partitioner_callback:(int -> string -> partition option)`](https://github.com/username_1/ocaml-kafka/pull/25/files#diff-359306da0b6a18ac454c04a7a53866c3R67) is correct.

However, trying to test it, I noticed that the integration tests are buggy and that the partitioner callback is not invoked. I will fix that independently of this PR which looks good.
comment