Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't treat Kafka connection errors as Makara database errors #243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

balvig
Copy link

@balvig balvig commented Aug 7, 2019

We use Kafka alongside Makara in our app, and for some Kafka events we wrap them in a transaction as we want to roll back any changes to the database in case an event doesn't get published.

When event publication fails, a Kafka::DeliveryFailed - Could not connect to any of the seed brokers exception is raised:

ApplicationRecord.transaction do
  record.save
  publish_event #  may raise Kafka::DeliveryFailed - Could not connect to any of the seed brokers:
end

The decription Could not connect to any of the seed brokers however ends up catching this and re-raising it as a Makara::Errors::AllConnectionsBlacklisted exception, due to the line here:

https://github.com/taskrabbit/makara/blob/dac6be2e01e0511db6715b2b4da65a5490e01cba/lib/active_record/connection_adapters/makara_abstract_adapter.rb#L23

...causing some difficulties with the way we track and manage exceptions.

This PR rather naively removes could not connect from the list of CONNECTION_MATCHERS, since all specs still seem to pass, but I'm not sure if there is something I'm unaware of! 🙇

The two errors that could have been affected, are actually caught thanks to the presence of connection refused in their descriptions:

https://github.com/taskrabbit/makara/blob/d609178c3ae278ba377efa2bbdaf10e4f4d71b6a/spec/active_record/connection_adapters/makara_abstract_adapter_error_handling_spec.rb#L37

https://github.com/taskrabbit/makara/blob/d609178c3ae278ba377efa2bbdaf10e4f4d71b6a/spec/active_record/connection_adapters/makara_abstract_adapter_error_handling_spec.rb#L41

Let me know if I've got the wrong end of the stick! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant