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 add error_cb to confluent.Consumer config in ConsumerFromTopic #44307

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

Conversation

SuccessMoses
Copy link
Contributor

closes: #43569


def error_callback(err):
"""Handle kafka errors."""
print("Exception received: ", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we try to use logger instead of a print here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think in other operators they use logger instead

Copy link
Collaborator

@rawwar rawwar left a comment

Choose a reason for hiding this comment

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

I think, the approach looks good and similar to what I have tested. Please update tests.

pass


def error_callback(err):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks promising!

error_cb(kafka.KafkaError): Callback for generic/global error events

We have to filter only related one ( https://docs.confluent.io/platform/current/clients/confluent-kafka-python/html/index.html#pythonclient-kafkaerror ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this what you meant?

def error_callback(err):
    """Handle kafka errors."""
    if err.code() == KafkaError.NO_ERROR:
        return
    else:
        print("Exception received: ", err)
        raise KafkaAuthenticationError(f"Authentication failed: {err}")

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we would like to catch only Authentication issue while error_callback consume all errors (https://docs.confluent.io/platform/current/clients/confluent-kafka-python/html/index.html#pythonclient-kafkaerror).

@@ -36,6 +48,7 @@ def __init__(self, topics: Sequence[str], kafka_config_id=KafkaBaseHook.default_
self.topics = topics

def _get_client(self, config) -> Consumer:
config["error_cb"] = error_callback
Copy link
Contributor

Choose a reason for hiding this comment

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

It might not be the best practice to update user-provided data. As an alternative, we could create a shallow copy instead.

Additionally, we can assume that the user has already provided an error_cb. In this case, we need to decide how to handle this behavior, which is an open question. One possible solution is to call the user-defined error_cb first and then invoke our custom implementation if the user does not raise an exception. This approach makes sense because authentication errors are not recoverable, so they should interrupt execution.

Copy link
Contributor Author

@SuccessMoses SuccessMoses Nov 25, 2024

Choose a reason for hiding this comment

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

Additionally, we can assume that the user has already provided an error_cb.

Is there a parameter for user defined error_cb?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, we can assume that the user has already provided an error_cb.

config may already contain handler. In this case you will override user defined handler.

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

Successfully merging this pull request may close these issues.

ConsumeFromTopicOperator does not fail even if wrong credentials are given
3 participants