[Resolve OOM When Reading Large Logs in Webserver] Refactor to Use K-Way Merge for Log Streams Instead of Sorting Entire Log Records #45129
Conversation
ed334f7 to
1d0e6ed
Compare
|
Rebased after we fixed main issue |
1d0e6ed to
8617e5b
Compare
0f19a8b to
ef3450b
Compare
|
CI is failing due to: Since the |
|
Applied and closed/reopened to trigger the build |
ef3450b to
0aaf0ab
Compare
|
Fix the provider tests that explicitly use the |
3aac539 to
46e30e3
Compare
46e30e3 to
1802ed1
Compare
|
Finally fixed the tests! This is the first (and likely the largest) PR for resolving OOM issues when reading large logs in the webserver. Even though the providers haven't yet been refactored to support stream-based log reading, the compatibility utility will transform the old For the testing part: |
1802ed1 to
091407e
Compare
- 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.
- Should handle input list is empty.
- Copy old test case that use read or _read methods - Add mark_test_for_stream_based_read_log_method and mark_test_for_old_read_log_method to skip corresponding CI tests
8aad65a to
8f89f5c
Compare
Thanks, @Lee-W, for reviewing! I’ve just resolved those nits. The CI failure is due to a flaky test: |
|
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. |
|
Since the TaskHandler logger being migrate to structlog, I will create another PR for the refactor instead of resolve conflict on this one( too much code change and conflict on this path recently) |
If that's the case, maybe we could mark this as draft or close and create a new one instead? |
|
Close this PR since it’s superseded by:
|
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.rstor{issue_number}.significant.rst, in newsfragments.