8000 fix(logger): Improve error handling to return errors when creating Kafka topics by deadlycoconuts · Pull Request #640 · caraml-dev/merlin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(logger): Improve error handling to return errors when creating Kafka topics #640

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 22, 2025
Merged

fix(logger): Improve error handling to return errors when creating Kafka topics #640

merged 1 commit into from
Apr 22, 2025

Conversation

deadlycoconuts
Copy link
Contributor
@deadlycoconuts deadlycoconuts commented Apr 22, 2025 •
edited
Loading

Description

It appears that calling CreateTopics here in NewKafkaSink sometimes doesn't return errors within the err return value, but instead in each of the individual topicResults (this is a slice) elements returned.

This happens in certain scenarios, e.g. when there are authorisation problems creating new topics, which causes errors to be returned within each element of the topicResults slice instead (leaving err as nil instead):

Screenshot 2025-04-22 at 15 19 52

This is problematic because it means that even though there were problems creating the topic, a nil error gets returned, with a nil sink value.

Tracing this back to where NewKafkaSink gets called in getLogSink, https://github.com/caraml-dev/merlin/blob/main/api/cmd/inference-logger/main.go#L362, and to where getLogSink gets called, https://github.com/caraml-dev/merlin/blob/main/api/cmd/inference-logger/main.go#L130, it's easy to see that the error isn't properly caught (since the error returned is nil) at any part of the call stack.

To make things worse, because logSink is nil, it continues getting passed around the entire logger code as if it were non-nil, creating huge problems downstream, like null pointer exceptions (and crashes) because logSink is not at all expected to be nil.

This PR simply refactors the error handling of the Kafka sink initialisation step, so that any errors are properly returned and subsequently caught by the upstream callers.

Modifications

  • api/pkg/inference-logger/logger/kafka_sink.go - Improve error handling

Tests

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes

None

8000
@deadlycoconuts deadlycoconuts added the bug Something isn't working label Apr 22, 2025
@deadlycoconuts deadlycoconuts self-assigned this Apr 22, 2025
@deadlycoconuts deadlycoconuts marked this pull request as ready for review April 22, 2025 07:29
@deadlycoconuts deadlycoconuts requested review from anantadwi13, andreas-aji and naufalandika and removed request for shydefoo and vinoth-gojek April 22, 2025 07:31
@deadlycoconuts
Copy link
Contributor Author

Thanks for the quick reviews everyone! 🚀 Merging this now!

@deadlycoconuts deadlycoconuts deleted the fix_error_handling_for_logger_topic_creation branch April 22, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreas-aji andreas-aji andreas-aji approved these changes

@vinoth-gojek vinoth-gojek vinoth-gojek approved these changes

@bthari bthari Awaiting requested review from bthari

@anantadwi13 anantadwi13 Awaiting requested review from anantadwi13

@naufalandika naufalandika Awaiting requested review from naufalandika

Assignees

@deadlycoconuts deadlycoconuts

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deadlycoconuts @andreas-aji @vinoth-gojek
0