Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve ElasticsearchTaskHandler #21942

Merged
merged 2 commits into from Jul 22, 2022
Merged

Improve ElasticsearchTaskHandler #21942

merged 2 commits into from Jul 22, 2022

Conversation

millin
Copy link
Contributor

@millin millin commented Mar 2, 2022 •
edited

Improve ElasticsearchTaskHandler:

  • use builtin logging.makeLogRecord instead of strange _ESJsonLogFmt
  • do not re-sort already sorted logs
  • apply ISO 8601 datefmt, because ES defines it as a datetime, not a text
  • fixed several found bugs

@millin millin force-pushed the improve_es_task_handler branch 2 times, most recently from c862386 to 9c58a04 Compare March 4, 2022 09:06
@millin millin marked this pull request as ready for review March 4, 2022 09:12
@millin millin force-pushed the improve_es_task_handler branch 2 times, most recently from 974635d to 09dde59 Compare March 4, 2022 11:44
@millin millin marked this pull request as draft March 4, 2022 13:43
@millin millin force-pushed the improve_es_task_handler branch 4 times, most recently from 385eb49 to 3a1460b Compare March 11, 2022 16:18
@millin millin marked this pull request as ready for review March 11, 2022 16:18
@eladkal eladkal mentioned this pull request Mar 14, 2022
Closed
@kurtqq
Copy link
Contributor

kurtqq commented Apr 4, 2022

This is a very needed fix. What is the status of the PR?

@millin
Copy link
Contributor Author

millin commented Apr 6, 2022

@ashb Hello! Could you please review this PR?

@millin millin force-pushed the improve_es_task_handler branch 3 times, most recently from e2b54ff to d02d6e0 Compare May 17, 2022 19:32
@potiuk
Copy link
Member

potiuk commented May 22, 2022

Looks like static checks/docs failing.

@potiuk
Copy link
Member

potiuk commented May 31, 2022

Some tests are still failing :(

@millin millin force-pushed the improve_es_task_handler branch 2 times, most recently from c6d61dc to c6345a8 Compare June 16, 2022 12:34
@potiuk
Copy link
Member

potiuk commented Jun 19, 2022

@potiuk I fixed the tests, but the build failed for unrelated reasons

Happens. Just rebase and let it re-run. I just did it - but please check if it succeed. and ping reviewers whwn it did (or fix it did not - sometimes the jobs fail intermittently and then re-running of just the failed job fixes it). Generally - you as the author should make sure it is green and ping reviewers when it is not, or where attempts to make it green failed.

@millin millin force-pushed the improve_es_task_handler branch 2 times, most recently from 710b890 to 4af6779 Compare June 21, 2022 14:12
@millin
Copy link
Contributor Author

millin commented Jun 21, 2022

@ryanahamilton @ashb @bbovenzi Please review

@potiuk
Copy link
Member

potiuk commented Jun 29, 2022

There is a conflict now to solve. The change looks good with regards to it's "interfacing" with the airlfow core, but I do not know that many details about Elasticsearch to be able to verify it fully. I think ES is used by Astronomer heavily, so maybe some more thorough review could be indeed be done by someone from the team :)

@millin
Copy link
Contributor Author

millin commented Jun 29, 2022

I need some time to see what changed in code, will try to resolve conflict in few days

@millin millin mentioned this pull request Jul 1, 2022
Merged
@rino0601 rino0601 mentioned this pull request Jul 3, 2022
Merged
@millin
Improve ElasticsearchTaskHandler:
d98beaf 
- use builtin logging.makeLogRecord instead of strange _ESJsonLogFmt
- do not re-sort already sorted logs
- apply ISO 8601 datetime format
- fixed several found bugs
@millin
Fix docstrings & small fix
5864fb9
@millin millin force-pushed the improve_es_task_handler branch 2 times, most recently from 23e0118 to 5864fb9 Compare July 18, 2022 06:14
@potiuk
Copy link
Member

potiuk commented Jul 18, 2022

Static check is failing

@millin millin force-pushed the improve_es_task_handler branch 2 times, most recently from 2817d2d to 5864fb9 Compare July 19, 2022 08:00
@millin
Copy link
Contributor Author

millin commented Jul 19, 2022

@potiuk I tried to inherit from newly added class airflow.utils.log.timezone_aware.TimezoneAware to reuse formatTime method, but providers test fails because it uses Airflow 2.2.0.
So I reverted this change and just resolved all conflicts.

@potiuk
Copy link
Member

potiuk commented Jul 19, 2022

@potiuk I tried to inherit from newly added class airflow.utils.log.timezone_aware.TimezoneAware to reuse formatTime method, but providers test fails because it uses Airflow 2.2.0. So I reverted this change and just resolved all conflicts.

Glad we added protection :)

potiuk
potiuk approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I would love someone from the committers to have elasticsearch experience to take a look.

@millin
Copy link
Contributor Author

millin commented Jul 20, 2022

@ryanahamilton @ashb @bbovenzi Please review

@potiuk potiuk mentioned this pull request Jul 21, 2022
Closed
2 tasks
@potiuk potiuk linked an issue Jul 21, 2022 that may be closed by this pull request
Airflow ElasticSearch provider issue #25177
Closed
2 tasks
@potiuk
Copy link
Member

potiuk commented Jul 22, 2022

Tested by @PatrykKlimowicz in #25177 . Merging.

@potiuk potiuk merged commit a25f8c3 into apache:main Jul 22, 2022
@potiuk
Copy link
Member

potiuk commented Jul 22, 2022

Thanks @PatrykKlimowicz for testing.

@millin millin deleted the improve_es_task_handler branch July 22, 2022 14:53
This was referenced Aug 10, 2022
Closed
Closed
@ephraimbuddy ephraimbuddy mentioned this pull request Aug 20, 2022
Closed
53 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@potiuk potiuk potiuk approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@ashb ashb Awaiting requested review from ashb

@bbovenzi bbovenzi Awaiting requested review from bbovenzi

@uranusjr uranusjr Awaiting requested review from uranusjr

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

Assignees
No one assigned
Labels
area:logging area:providers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Airflow ElasticSearch provider issue
3 participants
@millin @kurtqq @potiuk