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/03/20 16:48:11 UTC
[GitHub] [airflow] potiuk opened a new pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
potiuk opened a new pull request #22385:
URL: https://github.com/apache/airflow/pull/22385
The execution_date -> run_id change (#21960) attempted to make it
Airflow 2.1 backwards-compatible, but the problem is that in
Airflo2 2.1 retrieving `run_id` attribute of TaskInstance throws
AttributeError rather than returns None. It turns out that when
you have a field defined in an ORM model, it will never throw
AtributeError (even if you delete the attribute it will return
None.
Accesising `run_id` with getattr raises
AttributeError in Airflow 2.1 (because there TaskInstance has no
run_id defined). The test to test this behaviour is a little
convoluted to account for this behaviour of ORM models in
SQLAlchemy.
<!--
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] potiuk merged pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #22385:
URL: https://github.com/apache/airflow/pull/22385
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r834254247
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
Well. It certainly won't be worse than crashing when you try to send an elasticsearch log :)
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r832341591
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
Hey @jedcunningham @uranusjr @kaxil -> I'd really love to merge it and re-release the providers soon. The "extra packages" in the last list need to go away and the cncf.kuberbnetes starts to be a problem for users of Airflow 2.1 (likely this one https://apache-airflow.slack.com/archives/CCV3FV9KL/p1647936437392429)
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r831488721
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
@jedcunningham @uranusjr :pray: - I'd love to re-release providers asap as the ones we have install gitpython and wheel due to my sloppines :(
--
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 #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r831221736
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
Does this work with an empty string as a default?
cc @jedcunningham @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] github-actions[bot] commented on pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1075628103
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, 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] potiuk commented on a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830911178
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=ti.run_id if hasattr(ti, "run_id") else "",
Review comment:
I deliberately added `has_attr` here to make it explicit conditional. getattr(...., "") is shorter but kinda blurry to know if you've done that deliberately. So my check flags any "gettattr(ti, 'run_id'" - no matter if it has default or not.
This is a small thing, and I can be convinced otherwise, but I think it's better to promote `if hasattr` in this case.
--
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 pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1073289406
I need to re-release the providers due to bug in install_requires found (#22380 ) but before that, I would like to also get that one fixed (it was rasied by one of the users in https://apache-airflow.slack.com/archives/CCV3FV9KL/p1647519576375459
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830903878
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=ti.run_id if hasattr(ti, "run_id") else "",
Review comment:
we can't get run_id easily here in Airflow 2.1 case.
--
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 edited a comment on pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1073289406
I need to re-release the providers due to bug in install_requires found (#22380 ) but before that, I would like to also get that one fixed (it was raised by one of the users in https://apache-airflow.slack.com/archives/CCV3FV9KL/p1647519576375459
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830911178
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=ti.run_id if hasattr(ti, "run_id") else "",
Review comment:
I deliberately added `has_attr` here to make it explicit conditional. getattr(...., "") is shorter but kinda blurry to know if you've done that deliberately. So my check flags any "gettattr(ti, 'run_id'" - no matter if it has default or not.
This is a small thing, and I can be convinced otherwise, but I think it's better to promote if hasattr in this case.
--
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 #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830908376
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=ti.run_id if hasattr(ti, "run_id") else "",
Review comment:
`getattr(ti, "run_id", "")` is better; `hasattr` calls `getattr` anyway.
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830911178
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=ti.run_id if hasattr(ti, "run_id") else "",
Review comment:
I deliberately added `has_attr` here to make it explicit conditional. getattr(...., "") is shorter but kinda blurry if you've done that deliberately. So my check flags any "gettattr(ti, 'run_id'" - no matter if it has default or not.
This is a small thing, and I can be convinced otherwise, but I think it's better to promote if hasattr in this case.
--
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 pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1073846378
All green now.
--
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 pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1073733784
I also removed the unit test (it's not really needed now and it was pretty "convoluted" anyway).
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r831730939
##########
File path: airflow/models/xcom.py
##########
@@ -165,7 +165,10 @@ def set(
from airflow.models.dagrun import DagRun
if not exactly_one(execution_date is not None, run_id is not None):
- raise ValueError("Exactly one of run_id or execution_date must be passed")
+ raise ValueError(
+ f"Exactly one of run_id or execution_date must be passed. "
+ f"Passed execution_date={execution_date}, run_id={run_id}"
Review comment:
```suggestion
f"Passed execution_date={execution_date!r}, run_id={run_id!r}"
```
for readability
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r834254247
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
Well. It certainl won't be worse than crashing when you try to send an elasticsearch log :)
--
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 pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1074887792
Anyone ?
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830922586
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=ti.run_id if hasattr(ti, "run_id") else "",
Review comment:
Hmm. I convinced mysefl :).
It's better indeed, I think the default is "deliberate enough".
--
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 a change in pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830643282
##########
File path: airflow/models/xcom.py
##########
@@ -162,7 +162,10 @@ def set(
from airflow.models.dagrun import DagRun
if not exactly_one(execution_date is not None, run_id is not None):
- raise ValueError("Exactly one of run_id or execution_date must be passed")
+ raise ValueError(
Review comment:
I can remove those but they helped me to debug stuff.
--
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 #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r831221736
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
Does this work with an empty string?
cc @jedcunningham @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] potiuk commented on a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r834254849
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
This is what 2.1 users would experience if they run this provider version.
--
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 a change in pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830901372
##########
File path: airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
##########
@@ -283,7 +283,7 @@ def _get_ti_pod_labels(context: Optional[dict] = None, include_try_number: bool
return {}
ti = context['ti']
- run_id = getattr(ti, 'run_id') or context['run_id']
+ run_id = getattr(ti, 'run_id', None) or context['run_id']
Review comment:
You are quite ritght. Actually I figured that even for 2 months it is worth to add an extra protection and pre-commit to check it and - as it turned out there was one more case like that in elasticsearch which I also fixed.
--
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 #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r832617122
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
Approved the PR but I still don't know about this and haven't dug deep
--
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 a change in pull request #22385: Fix "run_id" k8s and elasticsearch compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r831234670
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
It's only formatting hte log entry so I "guess" it produces proper log entry. But good point - I do not know if the es logs will be properly parsed by ES engine then (though having unparseable logs is still infintely bettern than crashing airflow in this case :D)
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -129,7 +129,7 @@ def _render_log_id(self, ti: TaskInstance, try_number: int) -> str:
return self.log_id_template.format(
dag_id=ti.dag_id,
task_id=ti.task_id,
- run_id=ti.run_id,
+ run_id=getattr(ti, "run_id", ""),
Review comment:
It's only formatting hte log entry so I "guess" it produces proper log entry. But good point - I do not know if the es logs will be properly parsed by ES engine then (though having unparseable logs is still infintely better than crashing airflow in this case :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
[GitHub] [airflow] uranusjr commented on a change in pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#discussion_r830730423
##########
File path: airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
##########
@@ -283,7 +283,7 @@ def _get_ti_pod_labels(context: Optional[dict] = None, include_try_number: bool
return {}
ti = context['ti']
- run_id = getattr(ti, 'run_id') or context['run_id']
+ run_id = getattr(ti, 'run_id', None) or context['run_id']
Review comment:
I think this one’s mentioned in another PR; it should probably just be `context["run_id"]` since the key is available since basically forever.
--
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 pull request #22385: Fix cncf.kubernetes provider compatibility with Airflow 2.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22385:
URL: https://github.com/apache/airflow/pull/22385#issuecomment-1073315864
Looks like it works :)
--
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