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

Fix Kafka Client List Topics Method #88

Merged
merged 1 commit into from
May 3, 2024

Conversation

nasark
Copy link
Member

@nasark nasark commented May 2, 2024

This method uses the rdkafka admin API to retrieve metadata from the kafka server. This is a good way to perform health checks since if the kafka server is down, this will return an error instead.

@miq-bot assign @agrare
@miq-bot add_reviewer @agrare
@miq-bot add_label enhancement

@nasark
Copy link
Member Author

nasark commented May 2, 2024

We will need a new release of this gem once this is merged

Comment on lines 73 to 74
native_kafka = producer.instance_variable_get(:@native_kafka)
Rdkafka::Admin.new(native_kafka).metadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use ::Rdkafka::Config#admin method here rather than having to use instance_variable_get, https://github.com/karafka/rdkafka-ruby/blob/main/lib/rdkafka/config.rb#L263-L286

Suggested change
native_kafka = producer.instance_variable_get(:@native_kafka)
Rdkafka::Admin.new(native_kafka).metadata
@kafka_client.admin.metadata

@@ -69,6 +69,11 @@ def topics
Rdkafka::Metadata.new(native_kafka).topics.collect { |topic| topic[:topic_name] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasark I think we can use this method if we use:

Suggested change
Rdkafka::Metadata.new(native_kafka).topics.collect { |topic| topic[:topic_name] }
kafka_client.admin.metadata.topics.map { |topic| topic[:topic_name] }

@@ -69,6 +69,11 @@ def topics
Rdkafka::Metadata.new(native_kafka).topics.collect { |topic| topic[:topic_name] }
end

def metadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "metadata" is a little vague and would be hard to implement for the other messaging types, where listing queues/topics could likely be done.

I think we'd be better off fixing the "topics" method above, possibly implement an alive? method that catches Local: Broker transport failure (transport) (Rdkafka::RdkafkaError) and returns false and we could use that for the health-check, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we catch errors in the top level health check method which basically does the same thing https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_server.rb#L632, or is it better to catch here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we catch errors in the top level health check method which basically does the same thing

Yeah that's fine for now, I don't like exposing "internal" exception classes like RdkafkaError to the caller but we already do that elsewhere in this class. Ideally we'd catch RdkafkaError and wrap it in a ManageIQ::Messaging::Error class but that's a larger effort.

@nasark nasark force-pushed the fetch_metadata_from_kafka branch from 7d9446d to 54bfcf3 Compare May 3, 2024 16:04
@agrare agrare self-assigned this May 3, 2024
@agrare agrare added the bug label May 3, 2024
@agrare agrare changed the title Retrieve Kafka metadata method Fix Kafka Client List Topics Method May 3, 2024
@agrare agrare merged commit e7decd5 into ManageIQ:master May 3, 2024
11 checks passed
agrare added a commit that referenced this pull request May 3, 2024
Fixed:
- Fix Kafka List Topics Method (#88)

Added:
- Add ruby 3.1 to the test matrix (#84)
- Allow rails 7 gems in gemspec (#83)
@agrare
Copy link
Member

agrare commented May 3, 2024

Released v1.4.2 https://github.com/ManageIQ/manageiq-messaging/releases/tag/v1.4.2 with this change

@nasark
Copy link
Member Author

nasark commented May 3, 2024

Follow up #89

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

Successfully merging this pull request may close these issues.

2 participants