-
Notifications
You must be signed in to change notification settings - Fork 16.4k
DEMO: Enable shipping task log messages from anywhere #31241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This looks awesome. |
2fff1f5 to
fa599eb
Compare
fa599eb to
04d9489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably we can / should avoid creating the session if not using USE_PER_RUN_LOG_ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to above, I suspect that there is an optimization here, if USE_PER_RUN_LOG_ID is false, then we should not need to go to the database i think. this is probably not a huge issue on individual task logging i.e. on the worker but from something like scheduler you would probably want to avoid queries. though, not sure if it's really possible given the templating etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USE_PER_RUN_LOG_ID is not used here, and the case that USE_PER_RUN_LOG_ID == False is when users use airflow < 2.3.3. As I think we don't want to roll back the code to 2.3.3, I'm confused about what you mean by USE_PER_RUN_LOG_ID is false here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah as you mentioned on slack, i was just making quick conclusions based on what i saw in the code but it appears that this is not a global feature, just a backcompat flag in ES handler. but maybe we should make it a config and then we could use it here.
0f12304 to
69f01f9
Compare
|
Following up on this PR with some refactoring here: #32646 |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
demo
cc @RNHTTR