Bisk1 commented on code in PR #35712:
URL: https://github.com/apache/airflow/pull/35712#discussion_r1398008829
##########
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:
> Some of the connections also use additional fields
Http-based hooks don't use additional fields, some of them hide or re-label
fields but it's cosmetic - no functionality is missing without this.
> URI not the only one way to define connection, and to be honest not he
easiest way to create a connection
This is subjective. JSON format is newer but both have its uses. People who
specify connection with env variables will want a one-liner. And URI format is
still supported by Airflow. Also, JSON format has problematic names. Suppose
you want to set up connection to `https://example.com/endpoint`, then
configuration would be like this:
```
"example_https": {
"conn_type": "http",
"host": "https://example.com/endpoint";
}
```
but clearly `https://example.com/endpoint` is not a 'host', it's a URL so
you need to be experienced with JSON format to do it correctly. After the
change you could write it:
```
"example_https": {
"conn_type": "https",
"host": "example.com"
"schema": "endpoint"
}
```
(or in the URI format).
> This action is required to create additional Hook for handle this
"feature".
Can you elaborate what do you mean?
##########
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:
> Some of the connections also use additional fields
Http-based hooks don't use additional fields, some of them hide or re-label
fields but it's cosmetic - no functionality is missing without this.
> URI not the only one way to define connection, and to be honest not he
easiest way to create a connection
This is subjective. JSON format is newer but both have its uses. People who
specify connection with env variables will want a one-liner. And URI format is
still supported by Airflow. Also, JSON format has problematic names. Suppose
you want to set up connection to `https://example.com/endpoint`, then
configuration would be like this:
```
"example_https": {
"conn_type": "http",
"host": "https://example.com/endpoint";
}
```
but clearly `https://example.com/endpoint` is not a 'host', it's a URL so
you need to be experienced with JSON format to do it correctly. After the
change you could write it:
```
"example_https": {
"conn_type": "https",
"host": "example.com"
"schema": "endpoint"
}
```
(or in the URI format).
> This action is required to create additional Hook for handle this
"feature".
Can you elaborate what do you mean?
--
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]
