On Sat, Feb 29, 2020 at 2:02 AM Matt Buell <mattbuell...@gmail.com> wrote: > > Nice job Kamil, I really like where this is going - especially in regards > to Variable and XCOM functionality. > > With regards to the Task Instance and XCOM endpoints, I was wondering what > everyone's thoughts would be on using 'dag_run_id' as a unique identifier > as opposed to the existing 'execution_date'? From a client side > perspective, I think it would be easier to handle an ID versus a date. I'm > basing this off of the below Jira issue that says dag_id and run_id are > unique constraints (correct me if I'm wrong about this). I understand there > would also be compatibility issues between this change and the experimental > API. > > https://issues.apache.org/jira/browse/AIRFLOW-5593 >
I used id because it is the primary key of the table. I also looked and we have a uniqueness constraints also on other fields https://github.com/apache/airflow/blob/master/airflow/models/dagrun.py#L66-L67 execution_date sounds more sensible to me. In a moment, I will update the specifications. > > For the below methods, what about returning a list of IDs as opposed to the > full models? You can return a higher number of records with a smaller > amount data transfer. The user could then get the full model with > individual requests. > GET /dags > GET /dags/{dag_id}/tasks GET /dags This endpoint does not return any tasks. It only contains information from the database. It return following fields: dag_id, root_dag_id, is_paused, is_subdag, is_active, fileloc, owners, description, tags. GET /dags/{dag_id}/tasks Task_id is included here, but I think that creating views that allows you to fetch only a selected fragment of the object is an additional optimization that we do not need in the first version of the API. In the future, we can add an additional decorator that will add support for the fields parameter It may look similar to Google API -> Partial response https://developers.google.com/drive/api/v3/performance#partial-response > > Personally, I've never used connexion before, however, it seems quite > interesting to me. This yaml file would serve not only as a documentation > spec, but also as API configuration? I see connexion provides response > validation based off of the yaml configuration and ability to override the > validator. However - my concerns are similar to Ash's, how easy would it be > to maintain this yaml and maintain synchronization between docs and > functionality? > > https://connexion.readthedocs.io/en/latest/response.html#response-validation > If you want to make a change, you will first need to change the specification and then update the code. This is additional work, but it will allow us to avoid a situation that there will be a change in the code in the API that is not backward compatible. Maintenance should not be a problem, because you will first need to change the specifications before code change. You will not be able to change the code unless you change the specifications. The specification change will be reviewed in the same way as the code in the Pull Request. This way it will always be correct. > Best, > Matt > > On Fri, Feb 28, 2020 at 3:09 PM Ash Berlin-Taylor <a...@apache.org> wrote: > > > An easier URL to view the spec > > https://editor.swagger.io/?url=https://raw.githubusercontent.com/PolideaInternal/airflow/1e3e444d03c2e08d8fcee274d9e745a3d3ddeca8/openapi.yaml > > > > On Feb 28 2020, at 4:01 pm, Ash Berlin-Taylor <a...@apache.org> wrote: > > > To view the API spec < > > https://github.com/PolideaInternal/airflow/blob/1e3e444d03c2e08d8fcee274d9e745a3d3ddeca8/openapi.yaml> > > in an easier to consume form I used https://editor.swagger.io/ and > > imported from the (raw) github URL > > > Some comments on the proposed OpenAPI spec; > > > > > > connection id's are not unique, so get/delete might operate on multiple > > entries. (This has caused some confusion in the past, but is probably a > > separate issue to adding a REST API) > > > dag_id is listed as "integer" type. it should be string. Same applies to > > most other id types it appears. > > > > > > PATCH dag: fileloc shouldn't be writable (it's a security risk), > > is_active and is_subdag are an internal field and shouldn't be writable > > either. > > > > > > PATCH dag: - Did you think about/rule out having separate > > /dag/{dag_id}/pause and unpause methods? > > > > > > I don't think taskReschedule should be exposed directly at all - it's an > > internal detail for sensors in a specific state. > > > > > > PATCH variables - update_mask parameter: The way I've seen other APIs do > > this is PATCH vs PUT requests. PATCH should only update the specified > > fields, PUT is "here is the entire representation, make it look like this" > > > > > > PATCH variables - is_encrypted is exposed. It shouldn't be. It is not > > user-setable (it has is not writable in the UI anymore) > > > > > > GET xComValues: This endpoint support reading resources across multiple > > Task Instancce by specify a "-" as a !! - as a separator is not going to > > work. That is a valid character in task ids. > > > > > > xComValues -- I feel the "values" is out of place/not needed. > > > > > > > > > > > > -ash > > > (The thought of maintaining that swagger/openapi spec file is honestly a > > bit daunting and making me lean more towards adding spec validation to FAB > > than using connexion - specifically because it's a: huge and b: lives > > separately to the code/endpoints) > > > On Feb 28 2020, at 3:15 pm, Ash Berlin-Taylor <a...@apache.org> wrote: > > > > Great work Kamil, I can't wait till we have a fully featured API in > > Airflow! > > > > > > > > So AIP-13 has been voted on and "accepted". Do you have a proposal for > > what we should do with that AIP that we've already voted upon? > > > > In the permissions section you talk about creating, for example > > "can_edit_variable" -- but on which view? Permissions in FAB are a on a > > specific view -- see > > https://github.com/dpgaspar/Flask-AppBuilder/blob/120bc3ca38c4559a0099fe3b1a1badb319b6546e/flask_appbuilder/security/sqla/models.py#L71-L78 > > > > "There are not many FAB REST API experts" -- there aren't that many > > connexion experts either - has anyone involved with the project used > > connexion before?. It's a wildly inaccurate metric, but > > https://hugovk.github.io/top-pypi-packages/ lists FAB in position 980 and > > connexion in 1146, so they are both about as popular as each other. > > > > > > > > On Feb 26 2020, at 4:40 pm, Kamil Breguła <kamil.breg...@polidea.com> > > wrote: > > > > > Hello, > > > > > > > > > > I just created "AIP-32 - Airflow REST API" proposal and would love > > > > > community feedback and comments. > > > > > > > https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-32%3A+Airflow+REST+API > > > > > I would love to know what is your expectation from the API. > > > > > > > > > > We currently have one experimental API, but despite its existence for > > > > > 2 years, it has not reached a stable level. There are many critical > > > > > aspects to this implementation including no defined schema, very > > > > > narrow range of functions, unsafe default configuration, no HATEOAS, > > > > > and many others. > > > > > > > > > > In the past, Drewstone began work on REST API based on OpenAPI. It's > > > > > described by AIP-13: OpenAPI 3 based API definition. However, it was > > > > > not completed due to the lack of interest from the author and the > > > > > Kerberos configuration problem (It was at a time when Breeze was > > still > > > > > developing, so configuring all dependencies, including Kerberos, was > > a > > > > > problem). It had a narrow range of features that are expected by > > users > > > > > e.g. access to XCOM data and management for connection is missing, > > > > > > > > > > The Polidea and Google teams together with the community want to make > > > > > another attempt based on our and community experience. Airflow > > > > > deserves new stable solutions. > > > > > > > > > > Any comments and feedback/discussion in the AIP-32 document are > > welcome! > > > > > Best regards, > > > > > Kamil Breguła > > > > > > > > > > > > > > > >
