search

floydspace / effect-kafka

📨 Effectful Kafka
https://floydspace.github.io/effect-kafka/
MIT License
6 stars 0 forks source link

Add typed errors for production errors #15

Open anglinb opened 5 hours ago

anglinb commented 5 hours ago

The current signature for send is:

const send: (record: Producer.Producer.ProducerRecord) => Effect.Effect<Producer.Producer.RecordMetadata[], never, Producer.Producer>

I just ran into this error which caused the program to die:

[12:16:48.370] ERROR (#0):
  Error: Local: Queue full
      at Function.createLibrdkafkaError [as create] (/Users/brian/superwall/pwn-1/node_modules/@confluentinc/kafka-javascript/lib/error.js:456:10)
      at Client._errorWrap (/Users/brian/superwall/pwn-1/node_modules/@confluentinc/kafka-javascript/lib/client.js:552:29)
      at Producer.produce (/Users/brian/superwall/pwn-1/node_modules/@confluentinc/kafka-javascript/lib/producer.js:139:15)
      at /Users/brian/superwall/pwn-1/apps/consumer/node_modules/effect-kafka/lib/ConfluentRdKafkaInstance.js:51:56

(This was when using the RdKafka driver)

I think we should add a generic ProductionError the way RequestError and ResponseError work in the Http library.

floydspace commented 4 hours ago

@anglinb better imo ProducerError,

Would it be a good idea map to a more specific error like QueueFullException?

anglinb commented 4 hours ago

Looking into this more, seem like we might need to call poll, when using the ConfluentRdKafkaInstance

https://github.com/confluentinc/librdkafka/issues/4772#issuecomment-2306397276

anglinb commented 4 hours ago

@anglinb better imo ProducerError,

Yeah this is good with me!

Would it be a good idea map to a more specific error like QueueFullException?

I'm not sure, I guess it matters how you're going to handle it. It's seeming like the Exception is actually more related to not calling poll so I'm wondering if this is something you'd see frequently or not. If it's relatively infrequent (and we can fix the issue by calling poll after every produce, as indicated in some threads about the issue), then I'd opt for a generic error that maybe has some sub-types b/c then as a developer, I'm thinking. "Something unexpected happened" -> "retry policy", instead of QueueFullException -> "do something specific"?? I guess it just depends on how many things can happen. Lmk if that makes sense.

anglinb commented 4 hours ago

I also patched the library locally to call poll after every produce to see if it fixes the issue or not, we'll see 👀

floydspace commented 4 hours ago

then I'd opt for a generic error that maybe has some sub-types b/c then as a developer, I'm thinking. "Something unexpected happened" -> "retry policy", instead of QueueFullException -> "do something specific"??

I agree , if it can be handled there is no point to expose the error.