Same here, load balancing should be responsibility of the application itself, not Airflow.
+1 for making connections unique Bas On 11 Apr 2020, at 11:13, Robin Edwards <r...@bidnamic.com<mailto:r...@bidnamic.com>> wrote: On Sat, 11 Apr 2020, 01:20 QP Hou, <q...@scribd.com<mailto:q...@scribd.com>> wrote: It looks like we need to first make a decision on whether we want to enforce uniqueness for conn_id and implement connection load balance properly in another way. Yea I feel there are plenty of other tools and services for implementing load balancing and this particular feature doesn't really need to be provided by Airflow. I'd be in favour of making them unique. Also +1 on making audit log read only from API point of view. Thanks, QP Hou On Thu, Apr 9, 2020 at 9:47 AM Ash Berlin-Taylor <a...@apache.org<mailto:a...@apache.org>> wrote: They are, but we _can_ make a choice to remove that feature -- it is not widely used and is confusing to many when they stumble on it. It's not something we should do lightly, but it is a possibility. I think I'm probably leaning towards the "ordinal" concept: /connections/hdfs_default -> list of connections with that ID /connections/hdfs_default/0 first connection of that type Something like that. On Apr 9 2020, at 2:31 pm, Shaw, Damian P. <damian.sha...@credit-suisse.com<mailto:damian.sha...@credit-suisse.com>> wrote: FYI if you look back at the thread "Re: [2.0 spring cleaning] Require unique conn_id" on 2019-04-14 you can see a message from Kevin Yang stating that this random choice of connections is a "feature" used to load balance connections in AirBnB. So users are relying on this behavior. -----Original Message----- From: Daniel (Daniel Lamblin ) [Data Infrastructure] <lamb...@coupang.com<mailto:lamb...@coupang.com>> Sent: Wednesday, April 8, 2020 20:01 To: dev@airflow.apache.org<mailto:dev@airflow.apache.org> Subject: Re: API spec questions Having been bit by accidentally having two connections by the same name or conn_id, I'd prefer if were made unique. In my experience there's little utility in having multiple connections by the same name. Tasks that use a connection do to fairly randomly choose one, rather they seem pretty consistent in using the one created earliest, which often has the lower id integer. Circling back to how this is used by the API, from a user perspective, the following in path integer fields were ones I'd have expected to be strings instead: ConnectionID https://github.com/apache/airflow/blob/11f1e0cad996d5596e3e4fb440eb4ec52c024f70/openapi.yaml#L1845 DAGID https://github.com/apache/airflow/blob/11f1e0cad996d5596e3e4fb440eb4ec52c024f70/openapi.yaml#L1853 PoolID https://github.com/apache/airflow/blob/11f1e0cad996d5596e3e4fb440eb4ec52c024f70/openapi.yaml#L1901 Though it’s a url-encoding hassle, I also expected that DAGRunID would be more like the Run ID E.G. "scheduled__2020-04-08T23:10:00+00:00", "manual__2020-04-08T23:00:56+00:00", "manual__2020-04-08T16:24:56.692868+00:00" , "backfill_2020-04-08T22:05:00+00:00" etc Then TaskID is confusing to me; AFAIK the PK to task instances are task_id, dag_id, and execution_date and the api call appears to align with that having the pattern: /dags/{dag_id}/taskInstances/{task_id}/{execution_date}: But if task_id is a numbered id, then… execution_date isn't even needed… I'm thinking it should have been a string. An aside to this, I've always wondered what happens if an externally triggered DAG Run has the same execution date as a pre-existing scheduled DAG Run. They'd have different run_ids, EG "scheduled__2020-04-08T23:10:00+00:00" vs "manual__2020-04-08T23:10:00+00:00" but then task instances for those runs might not be unique. Lastly, the UI and CLI operation of clearing tasks seems analogous to the delete task instance API end point. But probably it's not, and this could become confusing to users. There should be a non-db-model api call for clearing tasks like you can from the UI and the CLI. If I read it right, clearing does the following: it sets the state of the task instance to None unless the state was Running then instead it sets it and related job ids to Shutdown. It deletes reschedules of the TI and it sets the dag runs for those task instances back to Running. This would be a lot to do for a user using PATCH calls to change Task Instances and Dag Runs together (and there's no API for Jobs). -Daniel L. P.S. as far as renaming all parameters on operators and hooks with *_conn_id, I do not want to see that breaking change happen. But IF it HAS TO, I'm still of the opinion that the default_args use for X_conn_id is not preferable to having all operators take simpler source_conn_id and optional target_conn_id parameter names that are consistent across all operators. On 4/8/20, 9:47 AM, "Ash Berlin-Taylor" <a...@apache.org> wrote: [Warning]: This email originated from an external source. Do not open links or attachments unless you know the content is safe. [경고]: 본 이메일은 회사 외부에서 유입되었습니다. 내용이 안전한지 확인하기 전까지는 링크나 첨부파일을 열지 마십시오. To expand on the "so I think we need to do one of": - we need to update the name of "conn_id" somehow. I can't think of a better option, and given all the operators have "x_conn_id" I don't think that change should be made lightly. - make conn_id unique (this "poor" HA has been a source of confusion in the past) and the ID we use in the API Or a third option: - Have the API take conn_id and either return all conns for that conn_id, or conn_id plus an ordinal (?idx=0 type thing) to return a single value. On Apr 8 2020, at 5:42 pm, Ash Berlin-Taylor <a...@apache.org> wrote: Hi everyone, So as I mentioned in the AIP voting thread, I think we need to give some more thought to how we are exposing connection ids in the API. Right now as proposed (and merged without approval, not cool. The AIP we voted on did not contain a PR against apache/airflow.) it has an end point of `/connections/{connection_id} ` My issue here is as I said in the previous thread: that is going to be mightly confusing to our users because there is a "conn_id" concept that is exposed, so people are going to try putting "aws_default" etc in there. I don't care what the API spec says, I care what our users expect, and having a connection_id/id and a conn_id fields is just confusing So I think we need to do one of: - we need to update the name of "conn_id" somehow, make conn_id unique (this "poor" HA has been a source of confusion in the past) There are similar problems for the DAG run -- the spec has the type as an integer, but everything else about the Airflow system uses the (unique) run_id parameter, and I would expect the API to use that. The autoinc. id on the run column is literally never used in the code base, so exposing that via the API seems odd. A few other smaller points: EventLog: Those are "audit"/action logs, so we probably shouldn't let people delete them via the API! pool_id: still an integer. It should be the "name". How should add/delete variables and connections work in the API with the addition of the new "Secrets Backends"? xcomValues: task_id is listed as an integer. -ash =============================================================================== Please access the attached hyperlink for an important electronic communications disclaimer: http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html ===============================================================================
