dashbitco / broadway

Concurrent and multi-stage data ingestion and data processing with Elixir
https://elixir-broadway.org
Apache License 2.0
2.43k stars 161 forks source link

NoopAcknowledger fails with ack key being set #302

Closed LostKobrakai closed 2 years ago

LostKobrakai commented 2 years ago

Is there a reason for the NoopAcknowledger to require a nil ack key? I would've expected it to just take any message and work.

josevalim commented 2 years ago

I don't think so... but the ack key is meant to be internal, no? Why is it changing?

LostKobrakai commented 2 years ago

I built a custom producer and I naively put a "useful" key I had at hand as the ack key instead of nil and it made the pipeline blow up. I don't think there's a good reason to explicitly have an ack key with that acknowledger, but I also don't see a good reason to limit the possible ack key values.

josevalim commented 2 years ago

But the NoopAck is only set when you ack immediately... and then we proceed to immediately set the ack_ref to nil. How is NoopAck set in your case?

LostKobrakai commented 2 years ago
  defp wrap_readings(readings) do
    Enum.map(readings, fn reading ->
      %Broadway.Message{
        acknowledger: {Broadway.NoopAcknowledger, nil, []},
        data: %{reading: reading}
      }
    end)
  end

Like that in my producer. Is that acknowledger not meant to be used that way? Given this one is an idempotent pipeline there's not really a need for acknowledging here.

josevalim commented 2 years ago

I see. So maybe we should have an API for adding it?

    acknowledger: Broadway.NoopAcknowledger.init(),
LostKobrakai commented 2 years ago

Hmm, while that would work I'm not sure how "discoverable" this is given there's no similar API for other (at least the one core one) acknowlegers. The documentation for CallerAcknowleger shows this:

acknowledger: {Broadway.CallerAcknowledger, {pid, ref}, term}

for how it's meant to be added. I wouldn't expect the need to switch to a function for using NoopAcknowledger from that point.

I could also see this resolved by documenting that the NoopAcknowledger requires a nil key, though imo it still be strange that a "noop" would require a certain kind of input to not fail.

josevalim commented 2 years ago

We should add a similar function to CallerAcknowledger then. :)