Bisk1 commented on code in PR #35712:
URL: https://github.com/apache/airflow/pull/35712#discussion_r1397943452
##########
airflow/providers/apache/livy/hooks/livy.py:
##########
@@ -575,10 +575,11 @@ def _generate_base_url(self, conn: Connection) -> str:
if conn.host and "://" in conn.host:
base_url: str = conn.host
else:
- # schema defaults to HTTP
- schema = conn.schema if conn.schema else "http"
+ scheme = conn.conn_type
Review Comment:
It could use connection in a different way, but this difference doesn't
change the connection type. It so happens that my org also uses Airbyte hook -
I just checked and we use `http` connection type (not `airbyte`) for
AirbyteHook and it works perfectly fine. It was accidental but it actually
makes sense to me - just because AirbyteHook has some custom logic on top of
HTTP connection doesn't make this connection anything else than HTTP because
that logic (hook) is a higher level of abstraction than connection.
I can see a couple of benefits now:
1) The tactical one is that this change fixes a bug
https://github.com/apache/airflow/issues/10913 where you can't correctly set
path for `http` connection if this connection is created from Airflow URI
because the path from URI is used as protocol in the hook.
2) More importantly - UX:
a) easier connection creation for new users - even though Airflow URI in
principle is not a valid HTTP(S) URI, users can expect if to be interpreted as
such for HTTP connections - nothing in the URI syntax in docs suggests that
connection to https://example.com should be defined http://example.com/https so
it's completely counter-intuitive
b) wouldn't it be easier for users to have fewer connection types to choose
in the drop-down list? In practice all they need to choose for those cases is
HTTP/HTTPS
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
