potiuk commented on code in PR #47563:
URL: https://github.com/apache/airflow/pull/47563#discussion_r1988601470
##########
providers/apache/kafka/src/airflow/providers/apache/kafka/hooks/base.py:
##########
@@ -70,6 +69,15 @@ def get_conn(self) -> Any:
and bootstrap_servers.find("cloud.goog") != -1
and bootstrap_servers.find("managedkafka") != -1
):
+ try:
+ from airflow.providers.google.cloud.hooks.managed_kafka import
ManagedKafkaHook
+ except ImportError:
+ from airflow.exceptions import
AirflowOptionalProviderFeatureException
+
+ raise AirflowOptionalProviderFeatureException(
+ "Failed to import ManagedKafkaHook. For using this
functionality google provider version "
+ ">= 14.1.0 should be pre-installed."
+ )
Review Comment:
Yes. We have a few cases where it's not easy to separate things out as we do
not have equivalent of "common.sql" as a common interface that we could plug-in
some "DBApiHook" to efectively cause a dependency-injection
This is the case with Apache Beam for example:
https://github.com/apache/airflow/blob/main/providers/apache/beam/src/airflow/providers/apache/beam/hooks/beam.py#L41C67-L41C87
In this case it's even "worse" because the google provider imports are
top-level and effectively apache.beam is not usuable in context outside of
google (however, this is not really an issue, because well, apache.beam is not
**really** usable outside of google context now - so it has not caused any big
issues so far.
With Kafka it's a bit different, because it is - of course - usable outside
of Google authentication and context.
So the way it is implemented now it has a conditional "what if we are in
google cluster". Howe to inject the authentication needed in a specific context
(this time google)
There are several solutions:
a) built in a common abstraction in Kafka Hook so that authentication
information can be injected. That includes functionally similar to DBapi
operators where authentication comes from "connection id" and gets injected by
instantiating the hook
b) or (way simpler) add if-else in the Kafka hook that will do proper
injection
the b) is what we have now, it's of course not perfect - 1) is better but
also way more complex and adds extra layers of abstraction. it's a simple way
of hard-coding possible options in the kafka Hook.
The question is:
1) do we have other examples where we can do similar thing?
2) is this is the right time to do the common abstraction?
I am a big fan of "if you want to make things reusable, you have to make it
usable first" -> I personally have no idea how such common abstraction on Kafka
Hook would look like for other cases (and even if there are other cases)
So my answer to both questions is:
1) No we have no other examples like that
2) I think it's to early because we do not have 1)
So for me b) is the right way to implement it now. And when we have other
examples we can think how to abstract away a common interface for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
