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.

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

WIP: [Resolve OOM When Reading Large Logs in Webserver] Refactor to Use K-Way Merge for Log Streams Instead of Sorting Entire Log Records #45129

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

jason810496
Copy link
Contributor

related: #45079


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk potiuk force-pushed the refactor/webserver-oom-for-large-log-read branch from ed334f7 to 1d0e6ed Compare December 21, 2024 08:32
@potiuk
Copy link
Member

potiuk commented Dec 21, 2024

Rebased after we fixed main issue

@jason810496 jason810496 force-pushed the refactor/webserver-oom-for-large-log-read branch from 1d0e6ed to 8617e5b Compare December 21, 2024 10:09
@jason810496 jason810496 force-pushed the refactor/webserver-oom-for-large-log-read branch from 0f19a8b to ef3450b Compare December 23, 2024 12:44
@jason810496
Copy link
Contributor Author

CI is failing due to: Please ask the maintainer to assign the 'legacy api' label to the PR in order to continue.

Since the get_log endpoint in both the legacy API and FastAPI uses the read_log_chunks method, it’s necessary to fix the endpoints and their corresponding tests.

@potiuk potiuk added legacy ui Whether legacy UI change should be allowed in PR legacy api Whether legacy API changes should be allowed in PR labels Dec 23, 2024
@potiuk potiuk closed this Dec 23, 2024
@potiuk potiuk reopened this Dec 23, 2024
@potiuk
Copy link
Member

potiuk commented Dec 23, 2024

Applied and closed/reopened to trigger the build

- refactor _read and read methods in FileTaskHandler
- refactor read_log_chunks method in TaskLogReader
- add check log_stream type utils
- fix type checking for
    - test_file_task_handler_when_ti_value_is_invalid
    - test_file_task_handler
    - test_file_task_handler_running
    - test_file_task_handler_rotate_size_limit
    - test__read_when_local
    - test__read_served_logs_checked_when_done_and_no_local_or_remote_logs
- also test compatible interface for test__read_served_logs_checked_when_done_and_no_local_or_remote_logs
    - which might call _read_remote_logs
- Since read_log_chunks is public method, refactor it as same return
  type in orignial implementation to avoid breaking change
- The `host` should only show once in read_log_stream
- Fix mock_read to new stream-based reading in test_log_reader
- Fix test for expecting stdout of callable should be in log lines
- Logs might not be add to heap in first round, should consider
  log_streams instead of heap
- Make it compatible for providers that implemented _read method.
@jason810496 jason810496 force-pushed the refactor/webserver-oom-for-large-log-read branch from ef3450b to 0aaf0ab Compare December 25, 2024 04:18
@jason810496
Copy link
Contributor Author

Fix the provider tests that explicitly use the read or _read methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging legacy api Whether legacy API changes should be allowed in PR legacy ui Whether legacy UI change should be allowed in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants