You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/20 15:54:07 UTC
[GitHub] [airflow] dstandish opened a new issue, #25186: should we normalize scheme and hostname of URI to be lower?
dstandish opened a new issue, #25186:
URL: https://github.com/apache/airflow/issues/25186
datasets are defined by their URI
we don't really spelly out what exactly our URI standard is, apart from reserving the `airflow` scheme and ascii
for websites, the scheme and hostname are case insensitive, while everything else is not.
should we normalize scheme and hostname or allow case sensitive differentiation?
my inclination is, we should allow everything to be case sensitive except possibly the scheme. and the reason is, we don't know exactly what "hostname" will mean for a dataset. if, for example, it's a database object, it could be case sensitive.
--
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-unsubscribe@airflow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1201801137
positive :)
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1201704598
Now we need to decide on what is the acceptable false-positive on duplicated URIs we can go with. This will hit us sooner or later, so it's quite necessary to make a conscious decision on it and accept the consequences, rather than leave it for interpretation.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190502403
> What about Airflow's Connection URI? We do not do any such normalization there. And we still call it a URI.
We do. We use urlparse which is RFC3986 compliant (and will do the normalization for us). https://docs.python.org/3/library/urllib.parse.html
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1201907758
Haha nice
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190511506
here's the PR: https://github.com/apache/airflow/pull/4591
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190627253
Actually again the RFC has interesting discussion result that explain possibilities with pros/cons. We are not the first to raise the question. There is chapter `6. Normalisation and Comparision` - especially "Equivalence" and "Comparision Ladder". Go read it @dstandish (and others who would like to comment). It's a very interstng read. We are not bound by the RFC rules, we just have to decide what level of false negatives we we are going to get.
There are two things that we heve not discussed - percent-encoding normalisation and path-segment normalization (all described in the RFC).
After reading it, and thinking a bit more - I think - personally we should normalise all: case, percent and path. I think we should not do "scheme"-normalisation (that would really require pluggable scheme normaliser eventually), and for sure we should not do "protocol" normalization.
Why?
The reason I think we should do it is because this is outward-looking data and because our URIs more often than not will be typed by users writing DAGs. Eventually what we are trying to do is to find out wheher two unrelated DAGs are operating on the same dataset. Any false negative there is a problem - not only for Airflow but also when the "lineage" data will be reported by Airflow to others.
If we want Airflow to become the source for other systems for lineage and dataset data in the future, we should I think take as much responsibility as possible to filter out false-negatives. People writing DAGs will perform silly mistakes and accidental casing mistakes will happen more ofen than you think (for example there is a very nasty key combination in vi that I sometimes trigger accidentally which uppercases whatever letter is underneath). If we do not normalise, this will mean that DAGs will work kinda properly. the dataset will be produced by source DAG, and it will be consumed by the Other DAG (for example s3://a, s3://A). But the link between those two DAGs will be broken. And they will not trigger one another. Maybe they will be triggered accidentally differently (if there also will be time trigger) - but the link will be lost and it's not going to be obvious why.
Not fully normalizing is fine for cache (Ok to have two copies) for example. But not for lineage.
IMHO - by not normalizing as much as we can, instead of solving the problem by automation (we can implement it, no problem, it's just complex), we are delegting the task to humans who are monitoring the DAGs. I think we will be much better in that task though than the humans looking at dags.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190504634
it doesn't though!
```
>>> from urllib.parse import urlparse
>>> urlparse('s3://Abc') == urlparse('s3://abc')
False
>>> urlparse('s3://abc') == urlparse('s3://abc')
True
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1191138623
If we are doing otherwise it is really equivalent to "we know you can make mistake, we could check it, but we won't do it - we want to throw the problem over the wall to other parts of the system to solve". The thing is that in general the sooner you detect problem, the cheaper it is to fix. By letting it spill-over we are making if far more expensive to fix.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190514510
Yep. It's a mess. But I think if we are about to be compliant with OpenLineage and others, being compliant with RFC. The worst we can do is invent yet another "our" interpretation of the standard:
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190506190
oh interesting:
```
urlparse('s3://abc').hostname
'abc'
urlparse('s3://Abc').hostname
'abc'
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190508331
Yep. This is what I was referring to - I believe I even fixed a bug some 2 years ago about RFC3986 compliance in Airflow. Can't find it quickly though
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190803026
It might actually be better to leave such normalization process to the lineage analysis platform. Something like a marquez or what have you. Because, for a project using a lineage tool, you may need to be able to synthesize your lineage data from multiple sources -- not just airflow but also spark or DBT or databricks or what have you. If airflow is "normalizing" the URIs -- in other words, taking the user-supplied string value, and changing it -- then you might get unexpected, and undesirable results.
Within an airflow cluster, if a dag depends on a dataset that airflow has no knowledge about, and it's due to a false negative, then this is something that we can actually warn the user about. Normalizing can actually produce a messier situation, because you allow users to have code that is inconsistent with the data (i.e. the URI value as registered in airflow) and this is not a good situation in a data engineering project. E.g. if you are trying to search _the code_ for references, you might not find them.
What I would be more ok with is throwing an error when Dataset is given a URI with a hostname or scheme thas not already lowercase.
```
def validate_hostname(val):
parsed = urlsplit(val)
hostname = parsed._hostinfo[0]
if hostname and hostname != hostname.lower():
raise ValueError("hostname must be lowercase")
def validate_scheme(val):
i = val.find(':')
if i > 0:
scheme = val[:i]
if scheme != scheme.lower():
raise ValueError("scheme must be lowercase")
```
This forces the code to be consistent with data, while not taking on a huge complexity burden.
Note we can't simply check `parsed.scheme` or `parsed.hostname` because they are already lowered.
Normalizing and reassembling the URI, particularly if you want to take on percent encoding and this kind of thing, would get very messy and force us to take on a lot of complexity that, at least to me, does not sound appealing, and does not seem like something airflow really needs to handle. With regard to false negatives, it's not fundamentally different from referencing another task ID in external task sensor or a dag id in trigger dag operator -- you have to get the reference right, and that's your responsibility as a user.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190503214
(It even has built in support for a number of schemas that implemented their own case-insensitive components)
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190498704
I believe we should just follow the standards (also because if anyone would like to use any tools to compare the URIs for example might use a tool that is RFC3986 compliant:
This is the URI's RFC: https://datatracker.ietf.org/doc/html/rfc3986 (not this is not HTTP URL RFC it's generir URI RFC so very much our case)
*Although schemes are case-insensitive, the canonical form is lowercase and documents that specify schemes must do so with lowercase letters
* The host subcomponent is case-insensitive
* Although host is case-insensitive, producers and normalizers should use lowercase for registered names and hexadecimal addresses for the sake ofuniformity, while only using uppercase letters for percent-encodings.
```
6.2.2.1. Case Normalization
For all URIs, the hexadecimal digits within a percent-encoding
triplet (e.g., "%3a" versus "%3A") are case-insensitive and therefore
should be normalized to use uppercase letters for the digits A-F.
When a URI uses components of the generic syntax, the component
syntax equivalence rules always apply; namely, that the scheme and
host are case-insensitive and therefore should be normalized to
lowercase. For example, the URI <HTTP://www.EXAMPLE.com/> is
equivalent to <http://www.example.com/>. The other generic syntax
components are assumed to be case-sensitive unless specifically
defined otherwise by the scheme (see Section 6.2.3).
```
The rest is case-sensitive by default. We can define some specific parts of the URI case insensitive but it dependes on scheme:
```
Some schemes define additional subcomponents that consist of case-
insensitive data, giving an implicit license to normalizers to
convert this data to a common case (e.g., all lowercase). For
example, URI schemes that define a subcomponent of path to contain an
Internet hostname, such as the "mailto" URI scheme, cause that
subcomponent to be case-insensitive and thus subject to case
normalization (e.g., "mailto:Joe@Example.COM" is equivalent to
"mailto:Joe@example.com", even though the generic syntax considers
the path component to be case-sensitive).
```
I think we do not have much freedom there in terms of host being case-sensitive. It cannot be.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190500415
> I think we do not have much freedom there in terms of host being case-sensitive. It cannot be.
What about Airflow's Connection URI? We do not do any such normalization there.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1191885294
100% on board.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190510272
yeah i think that line is probably what you're referring to ⬆️
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1201706757
er false negative, i think 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1201468634
lost the battle on this one @potiuk. still URI.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190509730
so if you call the hostname then it renders as lower. but maybe by doing that you are implicitly agreeing to that standard.
anyway we don't use this in airflow conn uri parsing. so we do _not_ normalize in this way and, in effect, treat it as case sensitive.
here it seems we pretty intentionally avoid use of urlparse.hostname for case-sensitivity reasons https://github.com/apache/airflow/blob/main/airflow/models/connection.py#L47-L50
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1190527854
yeah you are probably right. i'm not saying that we necessarily create a new standard. what's at stake for me is whether airflow takes on the responsibility of normalizing e.g. when storing in the database. in other words, given that the URI field is the unique identifier for an airflow dataset, does airflow really need to take on the responsibility of normalizing, such that when a user's code handles it in two different ways, we treat it as the same dataset. i'm reluctant to take on that responsibility, and i am more inclined to force the user to just make their code consistent. in other words, just store in a fully case-sensitivie collation, and not trouble ourselves with, e.g. decomposing, lowering hostname, recomposing and storing -- or, alternatively, storing hostname separately in a case-insensitive field and merge back in on read etc. i would rather not.
but i think we probably need to decide before 2.4 because a change to this behavior would be breaking.
unless we want to mark datasets as experimental... which i doubt... i digress
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1191135361
I think we should at least detect "fat finger" problems. I.e when somoene *Inside airflow installation* creates two different datasets with equivalent urls, we should not allow that. We are able to do do that very easily and warn the user.
I am perfectly ok with storing dataset with URL without normalisation. But at least we should have an unique index which will prevent the user from creating two different datasets with two equivalent (but different) URIs. This is not difficult. We can for example fully normalize the URI, convert into base64-encoded string and save it as "unique_id" or smth in the database. Then whenever we are inserting a dataset with different URI and same "unique_id", we simply fail with:
"This URI here is the same as that URI there". Shoudl not be very complex, and I think it prevents users from making silly errors that will be difficult to debug otherwise.
There is no real drawback of it that I can think except 'generate_unique_id" using normalisation. Pretty much no performance penaly, much better user experience. Airflow helping user to make less mistakes.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
dstandish commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1191854525
I am actually thinking now that we should abandon URI as the identifier for datasets. I think part of the motivation for choosing URI was [this document in openlineage](https://github.com/OpenLineage/OpenLineage/blob/main/spec/Naming.md). But the thing is, openlineage doesn't actually use URI for datasets! The document is misleading. In OpenLineage, what defines a dataset is actually `namespace + name`. To me, this is much better and I think we should change Dataset to do something more similar to that -- if not exactly that.
As we've seen, URI is problematic for a number of reasons. Just to name a few... It isn't "uniquely" defined by a string --- there are _many_ such URI that correspond to the same exact resource. It's not human friendly, and really requires that you _also_ have a friendly _name_ you can refer to it with (e.g. to present in UI). It can be confused for "connection string" -- when we're mearely looking for a label for a dataset. URI makes more sense as something generated by machines -- not as the human-entered label for a dataset. But openlineage has no need for it anyway. I plan to look at some other lineage standards to see how they identify datasets. But for now I am really feeling that we shouldn't use URI for this purpose.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #25186: should we normalize scheme and hostname of URI to be lower?
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #25186:
URL: https://github.com/apache/airflow/issues/25186#issuecomment-1201801507
I mean positive that it is false-negative, that is :D
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org