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/11/18 07:19:03 UTC
[GitHub] [airflow] humit0 opened a new pull request #19672: Fix log endpoint for same task
humit0 opened a new pull request #19672:
URL: https://github.com/apache/airflow/pull/19672
<!--
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/
-->
Log endpoint API raise exception when same `task_id` with different `dag_id`.
```
Traceback (most recent call last):
File "/opt/app-root/lib64/python3.8/site-packages/flask/app.py", line 2447, in wsgi_app
response = self.full_dispatch_request()
File "/opt/app-root/lib64/python3.8/site-packages/flask/app.py", line 1952, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/opt/app-root/lib64/python3.8/site-packages/flask/app.py", line 1821, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/opt/app-root/lib64/python3.8/site-packages/flask/_compat.py", line 39, in reraise
raise value
File "/opt/app-root/lib64/python3.8/site-packages/flask/app.py", line 1950, in full_dispatch_request
rv = self.dispatch_request()
File "/opt/app-root/lib64/python3.8/site-packages/flask/app.py", line 1936, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/_vendor/connexion/decorators/decorator.py", line 48, in wrapper
response = function(request)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/_vendor/connexion/decorators/uri_parsing.py", line 144, in wrapper
response = function(request)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/_vendor/connexion/decorators/validation.py", line 384, in wrapper
return function(request)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/_vendor/connexion/decorators/response.py", line 103, in wrapper
response = function(request)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/_vendor/connexion/decorators/parameter.py", line 121, in wrapper
return function(**kwargs)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/api_connexion/security.py", line 47, in decorated
return func(*args, **kwargs)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/utils/session.py", line 70, in wrapper
return func(*args, session=session, **kwargs)
File "/opt/app-root/lib64/python3.8/site-packages/airflow/api_connexion/endpoints/log_endpoint.py", line 64, in get_log
session.query(TaskInstance)
File "/opt/app-root/lib64/python3.8/site-packages/sqlalchemy/orm/query.py", line 3467, in one_or_none
raise orm_exc.MultipleResultsFound(
sqlalchemy.orm.exc.MultipleResultsFound: Multiple rows were found for one_or_none()
```
So, I add `dag_id` filter to get unique task_instance.
---
**^ 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] github-actions[bot] commented on pull request #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#issuecomment-975010439
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
[GitHub] [airflow] humit0 commented on pull request #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
humit0 commented on pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#issuecomment-972641337
> Good catch! But please don’t change the tests—duplication is somewhat intentional there.
>
> https://evgenii.com/blog/code-duplication-in-unit-tests/
You mean revert https://github.com/apache/airflow/pull/19672/commits/fbd054da53c1de872eee5ceb2bad357b37e9c83f commits? If so, I will revert that commit.
I use `RUN_ID` because other parts (`dag_id`, `task_id`) are using as variable when create API URL.
--
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] humit0 commented on a change in pull request #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
humit0 commented on a change in pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#discussion_r753899153
##########
File path: tests/api_connexion/endpoints/test_log_endpoint.py
##########
@@ -81,6 +81,18 @@ def setup_attrs(self, configured_app, configure_loggers, dag_maker, session) ->
configured_app.dag_bag.bag_dag(dag, root_dag=dag)
+ with dag_maker(
+ f'{self.DAG_ID}_copy', start_date=timezone.parse(self.default_time), session=session
+ ) as dummy_dag:
+ DummyOperator(task_id=self.TASK_ID)
Review comment:
@uranusjr OK! I will add some comments at that 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] humit0 commented on a change in pull request #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
humit0 commented on a change in pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#discussion_r753895685
##########
File path: tests/api_connexion/endpoints/test_log_endpoint.py
##########
@@ -81,6 +81,18 @@ def setup_attrs(self, configured_app, configure_loggers, dag_maker, session) ->
configured_app.dag_bag.bag_dag(dag, root_dag=dag)
+ with dag_maker(
+ f'{self.DAG_ID}_copy', start_date=timezone.parse(self.default_time), session=session
+ ) as dummy_dag:
+ DummyOperator(task_id=self.TASK_ID)
Review comment:
@uranusjr This is for adding new task which has different `dag_id` and same `task_id`.
--
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 #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#discussion_r753898435
##########
File path: tests/api_connexion/endpoints/test_log_endpoint.py
##########
@@ -81,6 +81,18 @@ def setup_attrs(self, configured_app, configure_loggers, dag_maker, session) ->
configured_app.dag_bag.bag_dag(dag, root_dag=dag)
+ with dag_maker(
+ f'{self.DAG_ID}_copy', start_date=timezone.parse(self.default_time), session=session
+ ) as dummy_dag:
+ DummyOperator(task_id=self.TASK_ID)
Review comment:
So the DAG is initialised so that we can be sure tests in the class can correctly pick up the correct logs? That makes sense to me, but it’d be best to add a comment describing why this is needed. Otherwise someone might ends up accidentally removing this as “unneeded code” in the future.
--
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 #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#discussion_r753892956
##########
File path: tests/api_connexion/endpoints/test_log_endpoint.py
##########
@@ -81,6 +81,18 @@ def setup_attrs(self, configured_app, configure_loggers, dag_maker, session) ->
configured_app.dag_bag.bag_dag(dag, root_dag=dag)
+ with dag_maker(
+ f'{self.DAG_ID}_copy', start_date=timezone.parse(self.default_time), session=session
+ ) as dummy_dag:
+ DummyOperator(task_id=self.TASK_ID)
Review comment:
What is this for?
--
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 #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#issuecomment-972603293
Good catch! But please don’t change the tests—duplication is somewhat intentional there.
https://evgenii.com/blog/code-duplication-in-unit-tests/
--
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 #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #19672:
URL: https://github.com/apache/airflow/pull/19672
--
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] humit0 commented on a change in pull request #19672: Fix log endpoint for same task
Posted by GitBox <gi...@apache.org>.
humit0 commented on a change in pull request #19672:
URL: https://github.com/apache/airflow/pull/19672#discussion_r753896351
##########
File path: tests/api_connexion/endpoints/test_log_endpoint.py
##########
@@ -81,6 +81,18 @@ def setup_attrs(self, configured_app, configure_loggers, dag_maker, session) ->
configured_app.dag_bag.bag_dag(dag, root_dag=dag)
+ with dag_maker(
+ f'{self.DAG_ID}_copy', start_date=timezone.parse(self.default_time), session=session
+ ) as dummy_dag:
+ DummyOperator(task_id=self.TASK_ID)
Review comment:
Does I need to create new testcase class for checking "Log endpoint API raise exception when same task_id with different dag_id."?
--
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