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 2021/12/23 23:36:27 UTC
[GitHub] [airflow] fbertos opened a new pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
fbertos opened a new pull request #20485:
URL: https://github.com/apache/airflow/pull/20485
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] uranusjr commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777820551
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -204,6 +207,10 @@ def get_dag_runs_batch(*, session: Session = NEW_SESSION) -> APIResponse:
else:
query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
+ if data.get("states"):
+ states = set(data["states"])
+ query = query.filter(DagRun.state.in_(states))
Review comment:
```suggestion
states = data.get("states")
if states:
query = query.filter(DagRun.state.in_(states))
```
Python variables are function-scoped, not block-scoped, so it’s not useful to put the variable inside the `if` block. And moving the variable out avoids one unnecessary member access overhead.
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776878687
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, I just did the change on the datatype to Optional[List[str]] as you mentioned but now when I try to run the static ckecks I am getting hundreds of errors (mypy). Is this normal?, what am I doing wrong? Thanks so much for any help.
cc @ephraimbuddy, @uranusjr
--
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] ephraimbuddy commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777856210
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
I don't think we need this duplication, we can use `_create_test_dag_run` instead. I understand you want to limit it to 2 dagruns but that's the essence of the PR, we can filter down to 2. `_create_test_dag_run` takes `state` argument. You can call it twice or more with different states thereby creating more dagruns and then test the added filtering.
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -307,6 +336,41 @@ def test_should_respond_200(self, session):
"total_entries": 2,
}
+ def test_filter_by_state(self, session):
+ self._create_test_dag_run_with_queued()
+ assert session.query(DagRun).count() == 2
+ response = self.client.get(
+ "api/v1/dags/TEST_DAG_ID/dagRuns?state=running,queued", environ_overrides={'REMOTE_USER': "test"}
+ )
+ assert response.status_code == 200
+ assert response.json == {
Review comment:
You may not need to check and list every fields. You can check important things like total returned, and states of the items returned
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -307,6 +336,41 @@ def test_should_respond_200(self, session):
"total_entries": 2,
}
+ def test_filter_by_state(self, session):
+ self._create_test_dag_run_with_queued()
Review comment:
```suggestion
self._create_test_dag_run()
self._create_test_dag_run(state='queued')
```
--
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] ephraimbuddy commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1000689082
> Can you add a test to filter by `none` state? I believe this is the state that causes the most problems (because the internal representation is not str). And there is actually an util function somewhere (for task instances? @ephraimbuddy may remember better) for this problem.
I think we are fine because we are dealing with dagruns which has only 3 states
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776878687
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, I just did the change on the datatype to Optional[List[str]] as you mentioned but now when I try to run he static ckecks I am getting hundreds of errors (mypy). Is this normal?, what am I doing wrong? Thanks so much for any help.
cc @ephraimbuddy, @uranusjr
--
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] fbertos commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1000724586
@ephraimbuddy @uranusjr also I got a failed result on static checks with this details but I do not understand:
black....................................................................................Failed
- hook id: black
- files were modified by this hook
Indeed I did not touch any hook in this PR, could you please help me to understand this issue? Thanks!
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777961017
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -307,6 +336,41 @@ def test_should_respond_200(self, session):
"total_entries": 2,
}
+ def test_filter_by_state(self, session):
+ self._create_test_dag_run_with_queued()
Review comment:
Hi @ephraimbuddy, understood this but we need to clarify the previous point before going further with this. Thx.
--
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] kaxil commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776853051
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -149,6 +149,7 @@ def get_dag_runs(
execution_date_lte: Optional[str] = None,
end_date_gte: Optional[str] = None,
end_date_lte: Optional[str] = None,
+ state: Optional[str] = None,
Review comment:
```suggestion
states: Optional[List[str]] = None,
```
cc @ephraimbuddy
--
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] kaxil commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776853078
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
```suggestion
if states:
query = query.filter(DagRun.state.in_(states))
```
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776860727
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, thanks for your recommendations. I will change the datatype to Optional[List[str]], however I am not sure it is a good idea to change to plural as the FilterState query component is defined in singular in the specification v1.yml, even being an array:
```
FilterState:
in: query
**name: state**
schema:
**type: array**
items:
type: string
required: false
description:
The value can be repeated to retrieve multiple matching values (OR condition).
```
To change that to plural I would need to create a new FilterStates query component....
On the other hand in the batch method I did use plural to match with the other existing fields (for instance dag_ids):
```
{
"order_by": "string",
"page_offset": 0,
"page_limit": 100,
"**dag_ids**": [
"string"
],
...
"states": [
"string"
]
}
```
--
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] boring-cyborg[bot] commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1000564756
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] fbertos commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1002715851
Hi @mik-laj, looks like this PR is pending of your review. Is there anything else I should check/change? This is my first PR so I am a little bit lost. Thank you for your help!!
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776860727
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, thanks for your recommendations. I will change the datatype to Optional[List[str]], however I am not sure it is a good idea to change to plural as it is defined in singular in the specification v1.yml, even being an array:
FilterState:
in: query
**name: state**
schema:
**type: array**
items:
type: string
required: false
description:
The value can be repeated to retrieve multiple matching values (OR condition).
On the other hand in the batch method I used plural to match with the other existing fields:
{
"order_by": "string",
"page_offset": 0,
"page_limit": 100,
"**dag_ids**": [
"string"
],
...
}
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r778131615
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
Ok @ephraimbuddy, let's try that. Thanks! I will try to minimize changes on the other parts of the code...
--
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] uranusjr commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777819686
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
As long as CI passes it should be OK (not optimal, but OK if you don’t want to fix those).
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777933839
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -204,6 +207,10 @@ def get_dag_runs_batch(*, session: Session = NEW_SESSION) -> APIResponse:
else:
query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
+ if data.get("states"):
+ states = set(data["states"])
+ query = query.filter(DagRun.state.in_(states))
Review comment:
Hi @uranusjr , thanks for your recommendation, but after that change I had this errors:
```
> states = set(data["states"])
E TypeError: 'NoneType' object is not iterable
```
So I put it back as the other lines (f.i. variable dag_ids, line 205) with the variable inside the block.
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776878687
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, I just did the change on the datatype to Optional[List[str]] as you mentioned but now when I try to run the static ckecks I am getting hundreds of errors (mypy). Is this normal?, what am I doing wrong? Maybe it is nothing to do with my change...? Thanks so much for any help.
cc @ephraimbuddy, @uranusjr
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r778131615
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
Ok @ephraimbuddy, let's try that. Thanks!
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777933839
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -204,6 +207,10 @@ def get_dag_runs_batch(*, session: Session = NEW_SESSION) -> APIResponse:
else:
query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
+ if data.get("states"):
+ states = set(data["states"])
+ query = query.filter(DagRun.state.in_(states))
Review comment:
Hi @uranusjr , thanks for your recommendation, but after that change I had this errors:
E TypeError: 'NoneType' object is not iterable
So I put it back as the other lines (f.i. variable dag_ids, line 205) with the variable inside the block.
--
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] uranusjr commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1004523494
One minor change, otherwise lgtm.
--
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] uranusjr commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r774883590
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -307,6 +307,42 @@ def test_should_respond_200(self, session):
"total_entries": 2,
}
+ def test_filter_by_state(self, session):
+ self._create_test_dag_run()
+ result = session.query(DagRun).all()
+ assert len(result) == 2
Review comment:
```suggestion
assert session.query(DagRun).count() == 2
```
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776860727
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, thanks for your recommendations. I will change the datatype to Optional[List[str]], however I am not sure it is a good idea to change to plural as it is defined in singular in the specification v1.yml, even being an array:
FilterState:
in: query
**name: state**
schema:
**type: array**
items:
type: string
required: false
description:
The value can be repeated to retrieve multiple matching values (OR condition).
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r778217947
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
Dear @ephraimbuddy, I just made a new commit with the changes. Please let me know if you agree with them. Thanks so much.
--
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] ephraimbuddy commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777853209
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -149,6 +149,7 @@ def get_dag_runs(
execution_date_lte: Optional[str] = None,
end_date_gte: Optional[str] = None,
end_date_lte: Optional[str] = None,
+ state: Optional[str] = None,
Review comment:
We have `state` in open API doc instead of `states`, I don't think it'll work to change it to `states`
--
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] fbertos commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1000710300
@uranusjr @ephraimbuddy thank you very much for your recommendations, I just made them in a new commit. Finally I did not add a test to filter by state None as @ephraimbuddy mentioned. Thanks
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776866273
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
After testing the change "Optional[List[str]]" I will need to change back the datatype to as with that change I can not pass the tests, errors detected in mypy checks:
airflow/plugins_manager.py:474: error: Incompatible types in assignment
(expression has type "Set[str]", variable has type "Optional[List[str]]")
attrs_to_dump = PLUGINS_ATTRIBUTES_TO_DUMP
^
airflow/plugins_manager.py:479: error: Item "None" of "Optional[List[str]]" has
no attribute "__iter__" (not iterable)
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776866273
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
After testing the change "Optional[List[str]]" I will need to change back the datatype to as with that change I can not pass the tests, errors detected in mypy checks:
airflow/plugins_manager.py:474: error: Incompatible types in assignment
(expression has type "Set[str]", variable has type "Optional[List[str]]")
attrs_to_dump = PLUGINS_ATTRIBUTES_TO_DUMP
^
airflow/plugins_manager.py:479: error: Item "None" of "Optional[List[str]]" has
no attribute "__iter__" (not iterable)
--
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] ephraimbuddy commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r774930134
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -307,6 +307,41 @@ def test_should_respond_200(self, session):
"total_entries": 2,
}
+ def test_filter_by_state(self, session):
+ self._create_test_dag_run()
+ assert session.query(DagRun).count() == 2
+ response = self.client.get(
+ "api/v1/dags/TEST_DAG_ID/dagRuns?state=running,failed", environ_overrides={'REMOTE_USER': "test"}
Review comment:
The failed state is not tested here. Can we fail one dagrun or set the state to queued and adjust the test to match
--
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] fbertos commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1001056377
@uranusjr, @ephraimbuddy all your guidelines were done. Is everything ok now or should I do something else? Thanks
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777933839
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -204,6 +207,10 @@ def get_dag_runs_batch(*, session: Session = NEW_SESSION) -> APIResponse:
else:
query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
+ if data.get("states"):
+ states = set(data["states"])
+ query = query.filter(DagRun.state.in_(states))
Review comment:
Hi @uranusjr , thanks for your recommendation, but after that change I had this errors:
> states = set(data["states"])
E TypeError: 'NoneType' object is not iterable
So I put it back as the other lines (f.i. variable dag_ids, line 205) with the variable inside the block.
--
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] boring-cyborg[bot] commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1005429527
Awesome work, congrats on your first merged pull request!
--
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] uranusjr merged pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20485:
URL: https://github.com/apache/airflow/pull/20485
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776860727
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, thanks for your recommendations. I will change the datatype to Optional[List[str]], however I am not sure it is a good idea to change to plural as it is defined in singular in the specification v1.yml, even being an array:
```
FilterState:
in: query
**name: state**
schema:
**type: array**
items:
type: string
required: false
description:
The value can be repeated to retrieve multiple matching values (OR condition).
```
On the other hand in the batch method I used plural to match with the other existing fields:
```
{
"order_by": "string",
"page_offset": 0,
"page_limit": 100,
"**dag_ids**": [
"string"
],
...
}
```
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777939436
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
Hi @ephraimbuddy , If we follow that approach we get these errors:
`E MySQLdb._exceptions.IntegrityError: (1062, "Duplicate entry 'TEST_DAG_ID-2020-06-11 18:00:00.000000' for key 'dag_run_dag_id_execution_date_key'")`
The problem is that the method _create_test_dag_run uses the self.default_time and self.default_time_2 as execution time statically. So when we call the method twice, we have a violation of PK.
To make this dinamically we should change also the way of assigning the execution dates...
How do you advice to proceed?
Thanks.
--
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] ephraimbuddy commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r778013667
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
What do you think about changing the method _create_test_dag_run to avoid that, if it's possible let's do it instead
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777939436
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
Hi @ephraimbuddy , If we follow that approach we get these errors:
`E MySQLdb._exceptions.IntegrityError: (1062, "Duplicate entry 'TEST_DAG_ID-2020-06-11 18:00:00.000000' for key 'dag_run_dag_id_execution_date_key'")`
The problem is that the method _create_test_dag_run uses the self.default_time and self.default_time_2 as execution time statically. So when we call the method twice, we have a violation of PK.
How do you advice to proceed?
Thanks.
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777939436
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -163,6 +163,35 @@ def _create_test_dag_run(self, state='running', extra_dag=False, commit=True):
session.add_all(dags)
return dag_runs
+ def _create_test_dag_run_with_queued(self, commit=True):
+ dag_runs = []
+ dags = [DagModel(dag_id="TEST_DAG_ID")]
+ dagrun_model_1 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_1",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='running',
+ )
+ dag_runs.append(dagrun_model_1)
+ dagrun_model_2 = DagRun(
+ dag_id="TEST_DAG_ID",
+ run_id="TEST_DAG_RUN_ID_2",
+ run_type=DagRunType.MANUAL,
+ execution_date=timezone.parse(self.default_time_2),
+ start_date=timezone.parse(self.default_time),
+ external_trigger=True,
+ state='queued',
+ )
+ dag_runs.append(dagrun_model_2)
+ if commit:
+ with create_session() as session:
+ session.add_all(dag_runs)
+ session.add_all(dags)
+ return dag_runs
Review comment:
Hi @ephraimbuddy , If we follow that approach we get these errors:
`E MySQLdb._exceptions.IntegrityError: (1062, "Duplicate entry 'TEST_DAG_ID-2020-06-11 18:00:00.000000' for key 'dag_run_dag_id_execution_date_key'")`
The problem is that the method _create_test_dag_run uses the self.default_time and self.default_time_2 as execution time statically. So when we call the method twice, we have a violation of unique index.
To make this dinamically we should change also the way of assigning the execution dates...
How do you advice to proceed?
Thanks.
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r777964974
##########
File path: tests/api_connexion/endpoints/test_dag_run_endpoint.py
##########
@@ -307,6 +336,41 @@ def test_should_respond_200(self, session):
"total_entries": 2,
}
+ def test_filter_by_state(self, session):
+ self._create_test_dag_run_with_queued()
+ assert session.query(DagRun).count() == 2
+ response = self.client.get(
+ "api/v1/dags/TEST_DAG_ID/dagRuns?state=running,queued", environ_overrides={'REMOTE_USER': "test"}
+ )
+ assert response.status_code == 200
+ assert response.json == {
Review comment:
Hi @ephraimbuddy, OK understood. I can change that according to recommendation. Thanks.
--
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] ephraimbuddy commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r774900110
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state == state)
Review comment:
```suggestion
if state:
query = query.filter(DagRun.state.in_(state))
```
The `FilterState` query component is an array.
--
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] uranusjr commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1000728056
You have trailing whitespaces in your code.
--
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] fbertos commented on a change in pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
fbertos commented on a change in pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#discussion_r776860727
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -164,6 +165,9 @@ def get_dag_runs(
else:
query = query.filter(DagRun.dag_id == dag_id)
+ if state:
+ query = query.filter(DagRun.state.in_(state))
Review comment:
Hi @kaxil, thanks for your recommendations. I will change the datatype to Optional[List[str]], however I am not sure it is a good idea to change to plural as it is defined in singular in the specification v1.yml, even being an array:
FilterState:
in: query
**name: state**
schema:
**type: array**
items:
type: string
required: false
description:
The value can be repeated to retrieve multiple matching values (OR condition).
On the other hand in the batch method I used plural to match with the other existing fields:
```
{
"order_by": "string",
"page_offset": 0,
"page_limit": 100,
"**dag_ids**": [
"string"
],
...
}
```
--
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] github-actions[bot] commented on pull request #20485: Add filter by state in DagRun REST API (List Dag Runs)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20485:
URL: https://github.com/apache/airflow/pull/20485#issuecomment-1004987757
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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