flipp-oss / deimos

Framework to work with Kafka, Avro and ActiveRecord
Other
59 stars 22 forks source link

Validate that producer is defined when including KafkaSource #90

Closed dorner closed 3 years ago

dorner commented 3 years ago

When a class includes KafkaSource but doesn't define a producer (by overriding kafka_producers or kafka_producer), we get a SystemStackError: stack level too deep. We should instead add a check to see if both methods are nil and if so, raise an error.

eduardopoleoflipp commented 3 years ago

What do you mean by:

check to see if both methods are nil

Do we mean to have some sort of top level validation at the parent class so that we fail explicitly in case of things like this:

def self.kafka_producers
end

The SystemStackError: stack level too deep is happening because here the owner that gets returned is Deimos::KafkaSource::ClassMethods and not Deimos::KafkaSource . I can put a patch for this but want to make sure I fully understand the bug description.

dorner commented 3 years ago

Honestly this was made as a backwards compatibility thing and we really should be moving all code to be self.kafka_producers. I'd actually be OK with removing the parent method self.kafka_producer entirely and changing this to a respond_to? check, logging a deprecation and removing calling the method in a future release.